MSPv2 support

[humanoid2050]

All of my changes.

[christianrauch]

Can you rebase on the upstream master? I.e. resolve the conflicts.

[humanoid2050]

I think that did it

[christianrauch]

Ok, merging would work now. But rebasing is still not possible: This branch cannot be rebased due to conflicts. Can you squash some of these commits together? E.g. there are a lot of "bugfix" and "remove dead code" commits. Since this is a feature PR, let's only commit the bugfixed version of the changed classes.

[humanoid2050]

Exactly what do you want to rebase onto what?

[christianrauch]

Rebase your branch (MSPv2_support) on the upstream (i.e. this repo) master. I.e. do:

git checkout master
git pull #(from my upstream repo)
git checkout MSPv2_support
git rebase master

[humanoid2050]

hmmm... thought I did that for this PR. I will try again...

[humanoid2050]

Rebased but not squashed.

[christianrauch]

That's a lot of changes :-) This will take a while to review. But thanks for taking the time to make this PR for MSP2.

I added some comments inline as a review. I still need to have a further look into the implementation and test it with my FC.

Some general comments about the code style:

• remove multiple blank lines, e.g. a single blank line is usually enough
• use CamelCase for classes and methods
• use CameCase for source and header files that define a class, e.g. if the class is called PeriodicTimer, let's call the source and header PeriodicTimer.cpp / PeriodicTimer.hpp
• in general try to go with the code style and naming conventions that are already used

More general comments:

• If a class or method is only used in one place, i.e. it is not included in multiple files, define the class in the same header file.
• Many methods are missing doxygen comments for documenting their in and outputs and the usage.
• Some classes that are only used by class Client have been used outside of the source file. When creating these helper classes (time and subscription) I added them directly to the same source file to (1) keep the file structure tidier and (2) also to not create private header files that are not to be used by a user of the library, i.e. to better structure private and public headers / API.
• Is this backward compatible to MSP1, i.e. a FC target that only supports MSP1?
• The README needs to be updated to reflect the new API.
• I get a lot of compiler warnings (gcc 7.3) about extra ‘;’ and unused parameters. Please fixe these.

[christianrauch]

By the way, squashing the commits is not really required. If you add new commits, the timeline will get very chaotic and I will probably squash these commits anyway. You are however free to squash the commits as it makes sense to you (e.g. one commits for messages, one for communication, ...) and force push to this PR branch.

[humanoid2050]

I will let you finish going through the diffs before I start moving things. I'm fine with most of the changes suggested so far. The only one I have any real feelings on is the collapsing of multiple classes into a single file. I can do that, but I find that my life is easier when a class is defined in files of the same name, especially if I don't know a priori that the use of that class is limited to another class. This code should be MSPv1 compatible. Part of the initialization is identifying the MSP version.

[christianrauch]

I had a further look. I think I am done with the code review for now. General comments:

• There seems to be a lot of output on the terminal from the Client class. Could you reduce this output to a minimum and use cerr for error messages?
• If a method parameter is not changed within a method, I am declaring them as const to make it clear to the user that this is an input and not an output.
• How does the MSP1 <-> MSP2 compatibility work? Is it possible to send messages in one version and get responses in a different version? Is there a version negotiation taking place?
• Can you explain again what the class msp::value is used for?

I still hadn't the chance to test it with the FC. But let's first get the code in code shape and make decisions on some of the changes.

[humanoid2050]

I will reduce the chatter. It's leftover from debugging. I can take care of const and formatting issues. In terms of V1 vs V2, the Client lib starts by assuming V1. There is a setVersion method to tell it to use V2. FlightController has a connect method which queries the FC to get its self-reported MSP version (over protocol V1), and then instructs the Client to use V2 if the FC is compatible. After that it stays using V2. The FC always responds in the version with which it was queried if I remember correctly. The msp::value class is used by the individual messages to hold the values unpacked from ByteVectors. The msp::value class also tracks whether the internal value has been set, which is useful when different firmwares have different fields in the payload. If we do something to separate message definitions into firmware specific versions, then the question of whether or not a particular possible field was set might go away.

[humanoid2050]

Oh, there is technically a MSPv2 over MSPv1 pattern, but the library doesn't deal with that.

[christianrauch]

There is this MSP2 over MSP1 encapsulation: https://github.com/iNavFlight/inav/wiki/MSP-V2#encapsulation-over-v1 but I don't think we need that. Apart from the M/X in the header, how do you determine the MSP version? I.e. I do not see in the code how this is set. I only see that you set the version via the M or X char in the header. But if the FC always responds with the same MSP version that a message was send, this will have no effect and msp_ver_ will never change from the initially set value.

[humanoid2050]

FlightController.cpp right around line 42.

[christianrauch]

Ups. I accidentally deleted my review (over 50 comments) before submitting it. I actually thought that you already receive some of my comments that I added to the diff. It will take a while until I can send a new review, sorry.

[humanoid2050]

Ouch. All good. I'm working from the comments you put in this thread at the moment.

[christianrauch]

So, I tried to recreate my comments. I was very brief this time and I probably forgot some.

[christianrauch]

Most comments are actually just about the code style. I might add a clang-format file with my desired code format. I am also happy if we keep every class in its own source/header file if these files have the same name as the classes (in CamelCase).

However, there are some things that effect the behaviour and API of the library and that need to be resolved before merging. Amongst others:

• location of message streaming operators
• periodic sending of cached RC values
• (feel free to add items to this list)

[humanoid2050]

So to address the functional concerns, I can move the stream operators, but I think the final form of the stream operators may be impacted by how we approach firmware variations in the messages. I may have broken MultiWii support, I definitely haven't done anything to validate it.

The automatic generation of MSP messages is a double edged sword. It can absolutely present a fly-away condition. I did it though because by default the ROS joy_node will not send updates if the input sticks don't move. I think I can adjust that and get ROS joy to send at a fixed rate, which moves the problem out of this library. I might like to leave the method in the library, just for scientific purposes even if it doesn't get automatically enabled as part of selecting MSP control. Does that sound acceptable?

The remaining outstanding question is what to do about the small handfull of messages which have different encodings between firmwares. I've been toying with the idea of making the message definitions have members matching the superset of all known fields across all firmwares. This means that the stream operators can work against a single definition, and some fields may just be unset if the particular firmware doesn't support it. The messages themselves could also have the firmware argument to the constructor removed. In order to get different packing/unpacking behavior in the messages, the encode/decode methods could be templatized with a FirmwareVariant. A default implementation can be provided, but if a particular firmware needs its own variation on the encode/decode, the template specialization for that variant just needs to be defined, possibly in a file holding all of the specializations for that firmware. The call then becomes

Ident::decode<FirmwareVariant::INAV>(ByteVector& data)

[christianrauch]

periodically sending of RC commands: The joy_node has a parameter autorepeat_rate to resend the last value (http://docs.ros.org/api/joy/html/). In any case, if someone uses a different node, he must make sure that RC messages are sent to the ROS node. It doesn't feel right to have something like this by default in the low-level access to the FC. If you want to demonstrate this functionality, you can also do this in one of the examples programs, e.g. by creating another thread that periodically sends default RC values.

different message definitions: I like the idea of having templated encode/decode methods. But then the firmware type needs to be known at compile time because you cannot change the template argument at runtime. At the moment, I am fine with having a single message definition for diverging messages, because there are only a couple of incompatible messages and they also just have a small number of different members. If message definitions diverge more over time, we should think about completely separating them in different namespaces.

[humanoid2050]

I will pull out the automatic MSP methods.

I think I need to comb through the code bases again and get an accurate count of the divergent messages. I did it before, but I for got the results.

[humanoid2050]

I've just about got the formatting changes done, so I started thinking about the issue of firmware variations. This may be a step too far, but what if the Message API was as deep as the user ever saw? There would still be specific internal classes for each message and each firmware variant, but it would all be opaque to the user and hidden behind getter/setter methods and a factory class.

class Message {
public:
    /**
     * @brief get the ID of the message (as sent to/from the FC)
     * @returns ID 
     */
    virtual ID id() const = 0;

    /**
     * @brief Queries the firmware variant the message expects to work with
     * @returns FirmwareVariant for this message
     */
    FirmwareVariant get_fw_variant() const
    {
        return fw_variant;
    }

    /**
     * @brief Decode message contents from a ByteVector
     * @param data Source of data
     * @returns False. Override methods should return true on success
     */
    virtual bool decode(ByteVector &data) = 0;

    /**
     * @brief Encode all data into a ByteVector
     * @returns Unique pointer to a ByteVector of data
     */
    virtual ByteVectorUptr encode() const = 0;

    /**
     * @brief Checks whether the underlying message has a particular field
     * @param String identifying the value to be retrieved
     * @returns True if the message knows about the value specified
     */
    template<typename T>
    virtual bool hasValue(std::string identifier) const = 0;

    /**
     * @brief Accesses a value set by the decode method
     * @param String identifying the value to be retrieved
     * @returns Copy of the Value<T> matching the identifier
     */
    template<typename T>
    virtual Value<T> getValue(std::string identifier) const = 0;

    /**
     * @brief Sets a parameter in the underlying derived message object. If the
     * parameter is not know, it gets ignored.
     * @param identifier String identifying the value to be set. Must already be known to 
     * derived type
     * @param val Value to be assigned to derived message
     * @returns True if successful
     */
    template<typename T>
    virtual bool setValue(std::string identifier, Value<T> val) const = 0;
};

[christianrauch]

What do you mean by:

was as deep as the user ever saw

The new Message definition that you provide has an additional get_fw_variant method. I am not sure if I understand how that is supposed work opaque to the user in the end. Are you proposing to have different classes for the same message type but different implementations of it?

[humanoid2050]

To answer you question, yes. Everything that needs to be unique or special ends up in a class derived from Message. The user just doesn't have to be aware of these specific classes. Take the Status message for example, ID 101. It has different data depending on which firmware packs it. So there would be the Message parent class, a derived Status class with all fields used by all firmwares, and InavStatus, ClflStatus, and MwiiStatus classes derived from Status. The lowest level classes would have the appropriate implementations of the encode/decode methods. From the user side, they just make a call to Factory::createMessage(MessageEnums::Status, FirmwareVariants::INAV) and get back a std::unique_ptr<Message>. So long as the requested combination of message and firmware was sensible to the factory, the unique_ptr is guaranteed to manage what is internally a very specific subclass of Message, in this case an InavStatus object. When the Client tries to unpack a buffer into the message, the virtual tables handle the selection of the appropriate decode method. The user then can extract information with message->getValue("cycle_time"). If the user is curious about the message internals (such as what firmware was used to populate its fields) they can get access to that. The combination of self-describing id and get_fw_variant methods can also be used by the print library to identify what the message actually is, and therefore what values it should expect to be able to access for printing. Otherwise the Message interface will need a std::vector<std::string> getAllFields() method that the print library can iterate over.

[christianrauch]

Ok. If we want to separate the message definitions per firmware, we would need to put them in their own namespaces, e.g. msp::msg::inav::Status and msp::msg::btfl::Status. However, if you want to instantiate the message from the correct namespace through a factory, you need to make this factory aware of all available messages. I can only imagine that you either define all messages for this factory manually or through some "magic" of template meta-programming or preprocessor macros. Alternatively, you could dynamically load libraries with message definitions. E.g. store all messages in dedicates libraries (shared objects) per firmware type and dlopen them based on the selected firmware. But this requires a "core" library of standard messages that can be used to actually determine the firmware type, and then loading the appropriate library. But this adds quite a lot of functionality to the msp library, which is just in place because forked firmwares cannot agree on message definitions :-)

However, for now, let's first merge the general MSP2 functionality to have the gross of the changes in place.

[humanoid2050]

I think the only remaining thing is the print library reorganization.

[humanoid2050]

I'm looking at the side effects of changing the printing. There's quite a bit that has to move, in terms of includes, linking, and code. I think it will also reduce overall capability of the library and increase the amount of work needed to maintain it and keep everything coherent. Especially if there are subsequent changes to the way multiple firmwares are handled. Also I'm getting a little bored.

[christianrauch]

That looks good. Can you click the "Resolve conversation" button for each of the addressed parts when you are done? It's easier to keep track of outstanding issues.

[christianrauch]

Regarding the formatted message printing, I see that this will introduce issues once we split the message definitions into separate namespaces.

Then, let's go with your approach of defining the printing within the message. For this, could you change the print method into a streaming operator? I.e. if we go this way, then it would be nice if the message could directly be used for standard I/O without needing to call the print function.

Also, I am thinking about having the streaming operator of the base class (Message) throwing "not implemented" exception by default, so that messages need to overwrite/implement that streaming function in order to make sure that messages that are used for standard I/O have a streaming operator.

[humanoid2050]

Stream operators have to be defined in the global scope. If they were defined in a class, then that would require the left hand argument of the operator be an instance of the class in question. As is, the left operator is ultimately an ostream object.

I made a couple other adjustments based on comments that I missed.

[christianrauch]

Why do the streaming operators need to be defined in the global scope? I found this: https://docs.microsoft.com/en-us/cpp/standard-library/overloading-the-output-operator-for-your-own-classes which declares the operator in the class and implements it in the source file. But in the end, it's still not clean as a method. So, it would make sense to go with your implementation.

[humanoid2050]

That refers to the "friend operator" technique. The method is declared in the class, but its still defined in the global name space. It just means that the operator can access the non-public members of the class when implemented.

[christianrauch]

Would it be possible for you to change the API examples in the README? E.g. since the MSP class is gone, the examples need to go too. The Client API changed a lot, but that's fine since it's not mentioned in the README. I think the fcu::FlightController API did not change, right?

[humanoid2050]

This is the last round of changes I will be making. I need to focus on other things.

[christianrauch]

I added a clang-format file and reformatted the code according to the settings. This should help to adhere to the code style. Just run find . -iname *.hpp -o -iname *.cpp | xargs clang-format -i --style=file in the root folder to autoformat all header and source files. I also fixed some of the compiler warnings and removed some more commented code and unnecessary output.

I am basically ready to merge this but would like to squash some of the commits (e.g. all "bugfix" commits) together. Would you like to squash the commits yourself? In any case, this would require to force push the MSP2 PR branch. If I do the squashing myself, can I directly force push the new history to this PR branch or shall I create a separate branch and a new pull request for this?

[humanoid2050]

I can squash things in this branch. Test programs all seem to run fine with INAV on my REVO board. Do you want all commits squashed down to where it split from your master branch?

[christianrauch]

Not everything. I thought mostly about bugfix commits, or commits that added something that was later removed:

• If you implemented something and fixed it in the following N commits, you could just squash these N bugfix commits into the commit that originally implemented something.
• If you added debugging stuff to the code and later removed it again - this could be squashed together, to easier track what was actually added.
• There are two commits with the very same name. If they changed the same parts in the code base, they could just squashed together.

[humanoid2050]

that's a little cleaner now

[christianrauch]

It's done! Finally! Thanks for the MSP2 support. Let me know (e.g., via e-mail) if you would like to work on a ROS1/ROS2 node for MSP communication.