Support for commands in parallel #85

julianoes · 2017-09-21T15:09:59Z

This is a big refactor of the MavlinkCommands class in order to support
multiple commands in parallel. It won't do multiple commands with the
same command ID at the same time in order to prevent conflicts/races
with acks that can't be assigned uniquely.

This is a simple wrapper around std::list to make it thread safe. The wrapper only implements a small subset of the features of std::list.

This adds some completion and errors/warnings for vim with the YouCompleteMe plugin.

ag (the-silver-searcher) did not ignore the directories specified in gitignore presumably because they were wrong.

This allows to change the timeout time of an existing (running) timeout.

This is a big refactor of the MavlinkCommands class in order to support multiple commands in parallel. It won't do multiple commands with the same command ID at the same time in order to prevent conflicts/races with acks that can't be assigned uniquely.

hamishwillee · 2017-09-22T00:08:15Z

.ycm_extra_conf.py

@@ -0,0 +1,206 @@
+# This file is NOT licensed under the GPLv3, which is the license for the rest
+# of YouCompleteMe.


See YouCompleteMe

Exactly - what!!

The text says "This file is NOT licensed under the GPLv3, which is the license for the rest of YouCompleteMe."

That looks like a placeholder to me.

And it should be obvious I am not actually reviewing this - I just wanted to see what the code looked like :-)

I can remove it and say it is licensed as BSD 3-clause like the rest of the repo.

Actually this file says it is public domain, so I'm not sure what I should do.

I thought this was our file. If YouComleteMe is an actual real name of a product called YouCompleteMe then we have to leave that alone - it isn't ours to modify.

Yep this file was copied from the YouCompleteMe project, we should leave as is

darioxz · 2017-09-25T11:43:21Z

core/log.h

@@ -101,6 +102,7 @@ class LogDetailed
                std::cout << "|Debug] ";
                break;
            case LogLevel::Info:
+                std::cout << ANSI_COLOR_BLUE;


This is really not critical, but #86 should get merged before #85

Is #86 rebased on to of #85?, can we remove this so we can merge w/o blocking?

mrpollo

I like this, I just have a few comments

mrpollo · 2017-09-25T18:50:00Z

core/log.h

@@ -101,6 +102,7 @@ class LogDetailed
                std::cout << "|Debug] ";
                break;
            case LogLevel::Info:
+                std::cout << ANSI_COLOR_BLUE;


Is #86 rebased on to of #85?, can we remove this so we can merge w/o blocking?

mrpollo · 2017-09-25T18:51:05Z

core/locked_list.h

+    class iterator: public std::list<T>::iterator
+    {
+    public:
+        iterator(typename std::list<T>::iterator c, std::mutex &mutex) :


I know we are using single char variables elsewhere but we should move away from this, can we name this variable something more descriptive, please?

Ok 173ac1a.

mrpollo · 2017-09-25T18:53:16Z

core/mavlink_commands.h

-        FAILED
-    } _state = State::NONE;
-    std::mutex _state_mutex {};
+    static constexpr double DEFAULT_TIMEOUT_NORMAL_S = 0.5;


We need to find a way to get the default timeouts into the documentation (if they aren't already there), this is important behavior that should be visible to external developers

@julianoes @mrpollo Arguably some timeouts should actually be part of the settable public API - and if so they could then be documented in the same way as everything else (?).

@julianoes Are all timeouts named consistently so we can find them? (i.e. can I have a list)

I've added mavlink/MAVSDK-docs#30 for tracking

Ok, I agree. Let's check this later.

mrpollo · 2017-09-25T18:57:39Z

core/mavlink_commands.h


    struct Work {
-        int retries_to_do = 3;
-        double timeout_s = 0.5;
+        int num_sent = 0;


can we use a more descriptive name here? is num_set the number of times a command has been set after retries? I tried following the code and got a bit confused

It is num_sent, not num_set but I've changed it to num_command_sent in a746eab.

mrpollo · 2017-09-25T18:59:12Z

core/timeout_handler.cpp

+    }
+}
+
+void TimeoutHandler::update(double new_duration_s, const void *cookie)


what about updated_duration_s instead of new_duration_s?

Sure, thanks. See ea4cdb8.

julianoes added 8 commits September 20, 2017 16:20

core: Added new locked list

f64dd81

This is a simple wrapper around std::list to make it thread safe. The wrapper only implements a small subset of the features of std::list.

Added .ycm_extr_conf.py for vim YCM

1b3e4de

This adds some completion and errors/warnings for vim with the YouCompleteMe plugin.

gitignore: fix it so that ag understands it

ba5e9f4

ag (the-silver-searcher) did not ignore the directories specified in gitignore presumably because they were wrong.

log: make LogInfo blue

64fceb3

core: add update for TimeoutHandler

61c84cf

This allows to change the timeout time of an existing (running) timeout.

locked_list: commented that this is not being used

6565a55

async_hover: improve debug outputs

752bee3

julianoes requested a review from darioxz September 21, 2017 15:09

hamishwillee reviewed Sep 22, 2017

View reviewed changes

julianoes added 4 commits September 22, 2017 14:25

core: added commented out printf

4d9a39e

core: remove unneeded while (true)

ac024a1

core: comment out a printf

438ad05

core: don't forget to reset timeout flag

6e910a2

julianoes requested a review from mrpollo September 22, 2017 13:26

darioxz reviewed Sep 25, 2017

View reviewed changes

mrpollo requested changes Sep 25, 2017

View reviewed changes

hamishwillee mentioned this pull request Sep 25, 2017

Add documentation about default timeouts mavlink/MAVSDK-docs#30

Open

julianoes added 3 commits September 26, 2017 12:19

locked_list: use better variable name

173ac1a

core: improve variable name

a746eab

core: improve variable name

ea4cdb8

mrpollo approved these changes Sep 26, 2017

View reviewed changes

mrpollo merged commit a9f2199 into fix-android-promises Sep 26, 2017

mrpollo deleted the async-commands branch September 26, 2017 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for commands in parallel #85

Support for commands in parallel #85

julianoes commented Sep 21, 2017

hamishwillee Sep 22, 2017

julianoes Sep 22, 2017

hamishwillee Sep 22, 2017

julianoes Sep 22, 2017

hamishwillee Sep 23, 2017

julianoes Sep 25, 2017

hamishwillee Sep 25, 2017

mrpollo Sep 25, 2017

darioxz Sep 25, 2017 •

edited

Loading

mrpollo Sep 25, 2017

mrpollo left a comment

mrpollo Sep 25, 2017

mrpollo Sep 25, 2017

julianoes Sep 26, 2017 •

edited

Loading

mrpollo Sep 25, 2017

hamishwillee Sep 25, 2017 •

edited

Loading

julianoes Sep 26, 2017

mrpollo Sep 25, 2017

julianoes Sep 26, 2017

mrpollo Sep 25, 2017

julianoes Sep 26, 2017

		@@ -0,0 +1,206 @@
		# This file is NOT licensed under the GPLv3, which is the license for the rest
		# of YouCompleteMe.

Support for commands in parallel #85

Support for commands in parallel #85

Conversation

julianoes commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioxz Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrpollo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioxz Sep 25, 2017 •

edited

Loading

julianoes Sep 26, 2017 •

edited

Loading

hamishwillee Sep 25, 2017 •

edited

Loading