MSPv2 support

[humanoid2050]

To recap, iNAV (http://inavflight.com/) has implemented version 2 of the MultiWii Serial Protocol. Somewhere between Cleanflight and iNAV, messages have diverged a bit. This thread is for discussions related to adding MSPv2 support to this msp library.

I am proposing a design that allows the Client class to return pointers to self-identifying objects that mirror the specifics of the message in whatever version of MSP the FC happens to be speaking.

For discussion purposes, we can look at message id 4, BoardInfo. This MSP library expects the following fields:

    std::string identifier;
    uint16_t version;
    uint8_t type;

The iNAV FC software generates the BoardInfo message with the following code:

        sbufWriteData(dst, boardIdentifier, BOARD_IDENTIFIER_LENGTH);
#ifdef USE_HARDWARE_REVISION_DETECTION
        sbufWriteU16(dst, hardwareRevision);
#else
        sbufWriteU16(dst, 0); // No other build targets currently have$
#endif
        // OSD support (for BF compatibility):
        // 0 = no OSD
        // 1 = OSD slave (not supported in INAV)
        // 2 = OSD chip on board
#if defined(USE_OSD) && defined(USE_MAX7456)
        sbufWriteU8(dst, 2);
#else
        sbufWriteU8(dst, 0);
#endif
        // Board communication capabilities (uint8)
        // Bit 0: 1 iff the board has VCP
        // Bit 1: 1 iff the board supports software serial
        uint8_t commCapabilities = 0;
#ifdef USE_VCP
        commCapabilities |= 1 << 0;
#endif
#if defined(USE_SOFTSERIAL1) || defined(USE_SOFTSERIAL2)
        commCapabilities |= 1 << 1;
#endif
        sbufWriteU8(dst, commCapabilities);
        sbufWriteU8(dst, strlen(targetName));
        sbufWriteData(dst, targetName, strlen(targetName));

Clearly, things have changed in the message definitions. I think this means that objects representing decoded messages will need to be able to identify themselves according to version from which they were decoded. My first thought is to add fields to the msp::Message class for representing version (with getter and setter methods). The Client class could have a method returning a pointer to a message, and the program would query the message to identify the version of the particular message in question.

The downside of course is that the program using the msp library has to understand the relationship between version and message fields.

Input is welcome.

[christianrauch]

The MSP_BOARD_INFO has the same message definition for both MSP versions. The de-/serialisation functions for the payload should stay the same. This is actually required for the backward compatibility. The version field in MSP_BOARD_INFO refers to the hardware version of the board, not the MSP protocol version.

The only technical document I found so far is: https://github.com/iNavFlight/inav/wiki/MSP-V2. The message structure shows that the header only has marginal changes:

• change magic bytes from M to X
• additional uint8 header field flag
• change message id (function) from uint8 to uint16 to support up to 65536 message types

I think for implementing MSP v2, we only need to change the way the messages are generated and parsed from the payload. The payload de-/serialisation stays the same, but the header format needs to be adapted. Ideally, there would be a way to determine which MSP protocol version a FC is using (e.g. by sending a ping message and see if the FC responds to this version of the protocol).

[humanoid2050]

I just went through Cleanflight, Betaflight, and iNAV source code. They all encode the MSP board info message with more parameters than this library decodes. I think it still works though because the fields being decoded here are the same as the first 3 being encoded on the other side. As long as the decoding library doesn't explode if there are more payload bytes than expected, simply adding fields on the encoding side will not break backward compatibility. This might mean then that the board info message is a separate issue and not related to msp v1 vs v2.

As for detecting v1 vs v2, it can be done by doing a v1 query for MSP_API_VERSION. iNAV returns version 2.1. Betaflight returns 1.39. It looks like Betaflight might actually support MSPv2, it just doesn't advertise it yet. Another option is to just issue a message with magic byte X and see if you get a response. I'm more in favor of the deterministic query pattern, that's what i've done with the code on my computer here. The Client class defaults to v1 and can then be explicitly set by the FlightController class on initialize() to use v2 if appropriate. The flag is then used in a couple places where execution branches one way or the other for encode/decode functions.

[christianrauch]

Sorry, I misread the first part of your message. I am not sure where I took the 3 fields for MSP_BOARD_INFO the first time. Since there is no official documentation for the message type definition, I always need to determine the structure manually from the FC or GUI source code. It might be, that these message definitions change over time in the FC source code. However, the message type definition is independent from the MSP version, since the protocol only deals with the raw payload and does not care about the message structure.

For determining the MSP version, I imagine that you could simply send a MSP v1 message ($M) and check the message header of the response (ignoring the actual message content). After sending the first MSP v1 message, you then switch to MSPv1 if receiving $M, and MSPv2 when receiving $X.

Which deterministic query pattern do you mean? Do you mean that you want to determine the MSP version by the firmware version?

[humanoid2050]

I do an API query using a v1 M message, then switch to v2 X messages if specific criteria are met. The FC will respond to messages with whatever format the external device used, be it v1, v2, or v2 over v1 on a per message basis.

[humanoid2050]

I've got a draft MSPv2 implementation over in my fork if you want to check it out. https://github.com/humanoid2050/msp/tree/MSPv2_support

[christianrauch]

The behaviour looks like the one that I had in mind to determine the MSP version at runtime. Thanks for the work. There is a lot of duplicated code in your fork, and also some changes that I cannot relate to the changed protocol. I envisioned that we only need to change the message ID type and the behaviour in the functions sendData, crc and processOneMessage and switch the header formatting according to ver. Since both protocols are quite similar, I think that there is no need to have dedicated (and duplicate) functions for each MSP version.

Independent of this, do you know if all recent MSP firmwares (Cleanflight, Betaflight, INAV) support MSP version 2? If so, I would also be happy to drop MSP version 1 support to simplify the implementation.

[humanoid2050]

The code definitely needs to be cleaned up. It was just a first draft to see what is possible.

As for version support, it looks like Cleanflight is still v1 only. Betaflight looks like it has the code, but I don't think it supports v2 officially yet. iNAV has fully embraced v2. I haven't looked at Baseflight or Raceflight.

[humanoid2050]

Oh, another thing. Apparently iNAV has depricated the various receiver-related options from the features set (https://github.com/iNavFlight/inav/wiki/1.8.0-Release-notes). That has some impacts on how a program interfaces with the FC. I've been fiddling with the example arming program, and I haven't figured out how to get around the changes in all cases.

I was also thinking if there is a way to handle the serial device disappearing on reboot. I connect over usb, and this library freaks out when the /dev/ttyACM0 device disappears on FC reboot.

[christianrauch]

The method enableRxMSP is adding {"RX_MSP"} to and removing {"RX_PARALLEL_PWM", "RX_PPM", "RX_SERIAL"} from the feature set on the FC to enable the RX_MSP feature. It might not work as expected, if the name of the RX features change. If the RX types are not part of the feature set at all (as in your case), you additionally will need another message type to configure this.

I have tried rebooting the FC without reconnecting only via USB-serial adapter. But this is a very uncommon use-case. Usually, you don't want to restart the FC while you are communicating with it. If you try to communicate with the FC using the MSP library, it should throw an error. In this case, you need to reconnect manually.

[humanoid2050]

I've been digging and I think the solution to the first problem is the new MSPv2 message MSP2_COMMON_SET_SETTING (0x1004). It looks like it allows the remote to manipulate all settings over msp.

As for the second problem, I can't fully get around that for my application. I shouldn't normally have to use the reboot-inducing feature change message, but I'm bolting a raspberry pi zero into my quadcopter, and the only serial device I have left on the FC is the USB interface.

[humanoid2050]

Quick question... what exactly is the relationship between the Client and MSP classes? They seem to have a lot of overlap, to the point where I would have to implement mspv2 in both places.

[christianrauch]

The MSP class was the early version of the API. I am not using this class any more in my projects. The Client class is the one that is used in the high-level FlightController API. I actually planned to deprecate the old MSP class, but I think people are still using it because it is simpler. I would focus on extending the Client class with the MSP version 2 support for now.

[humanoid2050]

I just made an intermediate commit in my branch. I started adding new messages using your existing pattern, but I got bored of tracking/computing the deserialization offset by hand. Also I knew I was going to make a mistake sooner or later and it was going to be hard to track down. Soooo... I changed the ByteVector typedef to a std::vector subclass and added methods and state to do that work for me. I made a method called unpack that will extract the values from the raw buffer and do the appropriate byte operations, keyed off the argument type.

I also discovered that there are a very small handfull of MSP Request messages that need to pack an outbound buffer to modify the behavior of the FC as it packs the response. That doesn't seem to fit into the current pattern of strict request or response types. So that will have to be implemented in the future.

Anyways, let me know what you think. Sometimes I kind of go off the deep end when I start coding by myself.

[humanoid2050]

@christianrauch, I've been making updates for MSPv2 in my fork. My implementation has diverged quite a bit from yours at this point. Are you interested in folding some of that back into your project? Otherwise I may make a hard fork and start a new project from just my MSPv2 code.

[christianrauch]

I am still interested in MSP2 support. Does your fork break a lot of the message definitions and API?

I also thought about replacing the current API with a new one that unifies the synchronous (class MSP) and asynchronous API (class Client) that I have right now. So this might replace a lot of your changes again.

However, if we merge your changes into a common upstream version, other people would benefit from your features (and you could too use the upstream version again) and it will be easier to apply future fixes and changes. It might also be easier to transfer to the new API later.

If you want to merge a lot of changes, please provide them in separated feature-based pull requests.

[humanoid2050]

I actually removed the MSP class from my branch. My needs were best served by the FlightController and Client classes, so I took it out for the time being. If a completely synchronous implementation is desired, it could probably be brought back in a from that matches some of the internal changes I made.

In terms of the FlightController and Client classes, the biggest API change is that there is no longer a differentiation between request and response messages. MSPv2 blurred that line with messages that require data to be packed for sending and data to be unpacked upon receiving. So now I have a single base class msp::msg::Message that all messages inherit from. Every round of communication calls encode before sending and decode after sending. Virtual methods ensure the redirection to the appropriate version of encode/decode depending on the nature of the message. Sending a message uses a function signature bool sendMessage(msp::Message& message, const double timeout = 0) or bool sendMessageNoResponse(msp::Message& message) Should probably point out that the second signature actually just ignores any response, as opposed to instructing the flight controller to not respond. MSPv2 seems to have an undocumented ability to not respond to a message if the flag field of the header is set to 1. Haven't tested that though. The entire system top to bottom is aware of the firmware variant (INAV, Cleanflight, Betaflight, etc) that is running on the flight controller. Some messages have different fields depending on which firmware encodes it. I've made a few messages aware of the different variations, but I'm mostly testing against INAV. I realize that adding support for each variation could represent a lot of work and maintenance, but I'm not sure how else to handle it.

Other changes: Methods for printing message contents are now in the messages themselves. The message print library is gone. The handler method for unpacking a message from the serial device is not called on a buffer until the asio library detects that a full message is in said buffer. (Also prevents hanging if there are no communications to make the asio::read() method return) Separate code paths for MSPv1 and MSPv2 when packing and unpacking the final buffers going to/from the actual serial device. FlightController can periodically send MSP_SET_RAW_RC messages on its own. This means that the virtual stick inputs can be updated at a low rate (reduce bandwidth) but the flight controller will not disarm due to lack of RC input. That does make for a potential fly-away condition if other protections are not in place. General refactoring to more of a one file/one class pattern.

TODO and under consideration: I'm using the library inside of a ROS node to generate MSP control from what is ultimately a ROS joy_node. That ROS node uses about 5% cpu on a Raspberry Pi Zero W. It was 10% before I started filtering out a lot of my debugging output. I suspect the CPU load can be further reduced by replacing the PeriodicTimer with something that has a single thread and a queue of waiting callbacks. Then instead of having one thread per subscription, you have one thread that sleeps until the next timer in the queue expires. Callback executes, timeout value increments, callback gets shuffled back into the queue in the right location. Probably need to use a heap structure for sorting instead of an actual std::queue, but care probably needs to be taken to make sure that a single callback with a too-short period doesn't starve the rest of the queued stuff. I also added some components from the std::atomic library to make thread behaviors more robust. These should not be active for most of the programs execution, but I know they can add overhead.

[christianrauch]

I haven't really looked into MSP2 and the only information I got so far is from https://github.com/iNavFlight/inav/wiki/MSP-V2. Unfortunately, there is no proper protocol and message specification that cleanflight, betaflight and iNav agree upon (unlike the MAVLink protocol or ROS / DDS). E.g. the flag field description mentions "usage to be defined" and the only option in the documentation is 0xa5 for embedded MSP1 messages. And from the same documentation I did not gather at first instance that the behaviour of exchanging payload changed. E.g. it used to be that payload is only send in on direction, e.g. you either get payload from the FC or you send payload to the FC, not both at once.

Most changes you mention are reasonable. E.g. I also thought about removing the MSP class for a long time since it is synchronous which doesn't allow to request sensor data at a high frequency. But I would like to keep the printing functions in a separate source file and library since I consider them not part of the normal usage of the library. E.g. they are only used in the examples and I sometimes use them for debugging but I expect that people mostly will directly use the processed messages without printing them in the terminal. If the behaviour changed in a way that you described, e.g. requests now have corresponding responses and payload is exchanged in both ways in a single request-response-session, then I am also happy to just drop MSP1 support and go entirely for MSP2.

I am also actually using the library in a ROS1 node (https://github.com/christianrauch/ros-multiwii) and ROS2 node (https://github.com/christianrauch/multiwii_ros2). I also observed that on a Raspberry Pi Zero, the library consumes a lot of resources when requesting data at a high frequency. Partially, this comes from the design of the protocol as request-response, which requires to request each individual sensor reading.

For a new API I actually wanted to remove the timer functionality from the library and only use two dedicated threads for sensing and receiving messages. The library would then only provide asynchronous send and receive functions for single messages and the periodic call of those would need to be provided outside of the library, e.g. in the process loop of whatever framework a user is using (take the ROS2 timer https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/timer.hpp for example).

[humanoid2050]

Yes, the MSPv2 documentation is lacking. For the most part I'm actually using the INAV source code for reference. I did actually reach out to the different groups to try to get a handle on the message fragmentation, but all I got back was a lot of talk about not wanting to break backward compatibility.

I do agree that putting printing into its own library has a lot of value in terms of code size and binary size, i just moved it because I didn't want to have to mirror two components and keep changes up to date in two places. The other upshot was that I could make the print functions virtual, which meant that printing a message that didn't have an implemented print method just produced silence instead of a compiler error. Pros and cons each way, it just made my work faster.

I should probably elaborate a little more on the request/reply thing. Probably 99% of all messages are still compliant with the request xor reply messaging paradigm. It's the new MSP2_COMMON_SETTING message which break the pattern. It is designed to reach into the flight controller and get any parameter visible from the CLI. As such, it requires the name of the setting to be encoded on send, and the name along with the value (in the FC's internal data encoding) is returned.

Let me see if I can prep some transitional merge requests. I think I might be able to make a merge request around the removal of request/reply classes and inheritance from a single message class. That will also bring with it the ByteVector class I'm using to pack and unpack individual values from the raw buffers. I know you aren't supposed to inherit from STL containers, but I did it anyway. Otherwise I get to re-implement a lot of the vector methods. Let me know if you have strong feelings on that.

[christianrauch]

The MSP2_COMMON_SETTING then needs a proper implementation of a remote procedure call where requests have corresponding responses e.g. like services in ROS. I can imagine that future MSP message definitions will make use of this too.

I will postpone my changes then until we have merged your MSP2 support and every required feature.

[humanoid2050]

I'm prepping a merge as discussed. Do all of the test programs work for you? I'm getting odd behavior, but I can't rule out that my flight controller is the one doing something odd.

[christianrauch]

I haven't used the library with newer betaflight versions for a long time. But I did not encounter issues when using it the last time on the master branch. There might be incompatible behaviour between firmware types. Could you test the examples with your FC on the master branch and report any outstanding issues?

[christianrauch]

Implemented in #28.