search

christianrauch / msp

Implementation of the MultiWii Serial Protocol (MSP) for MultiWii and Cleanflight flight controller
http://www.multiwii.com/wiki/index.php?title=Multiwii_Serial_Protocol
GNU Lesser General Public License v3.0
73 stars 26 forks source link

possible bug in msp_msg.hpp #18

Closed humanoid2050 closed 6 years ago

humanoid2050 commented 6 years ago

I think I may have found a problem in msp_msg.hpp

The definition for SetRawGPS inherits from Request. I think it should inherit from Response shouldn't it? It has an encode method that matches the Response base class pattern.

christianrauch commented 6 years ago

You are right, SetRawGPS needs to inherit from Response. Programs using this message type should actually not compile, since SetRawGPS does not implement decode. But I've never used this message type, so the bug never occurred to me.

humanoid2050 commented 6 years ago

I can make the change and do a pull request if you like, or you can make the change directly. As a side question, would you be interested in changes I'm making to support MSP v2 for the iNAV fork of Cleanflight?

christianrauch commented 6 years ago

I am happy to accept all PR that fix issues with the library or add useful features. I have seen the work on MSP v2 in the betaflight roadmap. As I am stuck with betaflight 3.2 (F1 FC hardware) I cannot test this feature myself.

humanoid2050 commented 6 years ago

I've got a cc3d revo with an F4 and I'm running iNAV which has actually implemented MSPv2. The issue, as with most protocols that evolve, is that messages have changed (notably messages have gained fields). I was thinking about making a new client method which returns a pointer to a request-derived object. Then the method can allocate an object appropriate to the particular version of msp that the FC is speaking. This of course means that the object needs to be able to identify itself and that in turns means more fields in the classes and so on and so forth. Does that sound like too profound a change for you?

christianrauch commented 6 years ago

I haven't had a detailed look at the MSP v2 documentation yet. I thought it is basically keeping the MSPv1 messages for backwards compatibility, and extending the range of message IDs to uint16 to allow much more message types. For a client, it should be enough to determine the MSP version the FC is using, and change the header field appropriately.

Can you open a new issue for the MSPv2 feature to keep track of the discussion there?

humanoid2050 commented 6 years ago

yep yep.