Closed humanoid2050 closed 5 years ago
There are quite a lot of changes. I won't be able to address all of them now. But thanks for contributing to begin with :-)
My comments so far by looking on the diff:
ByteVector
need to have the additional methods?// coment
. (2) Could you remove some of the blank lines such that there are no more than a single blank line between methods/function/variables?Client
need to store the firmware type (member firmware
) or could this be done in the FlightController
class? I would like to keep the Client
class as generic as possible. I.e. the Client class doesn't know anything about message types, it just does the communication. The FlightController
on the other hand is actually determining the firmware type and all required metadata.RxMap
), the firmware type is given via the constructor but it appears to me that it is never used within this message.print
method, and we have the stream operator defined in msg_print.hpp
. What is the advantage of the print method over the operator? Can we remove the print method from the messages and use the dedicated stream operators for printing messages?I decided to but the message IDs (enum msp::ID
) in its own header so that it can be available without loading all the message definitions. We could use a forward declaration of the enum class msp::ID
in message.hpp
and move the IDs into the header for the message definitions (msp_msg.hpp
). This would gather all the message definition stuff that might change with firmware in a single header file.
I will test your changes later on the most recent betaflight release.
Edit: For future pull requests, I would recommend to use a dedicated feature branch (with a reasonable name) and keep your master branch in sync with the upstream project.
To address your comments,
std::make_unique
. They forgot to put that into c++11.ByteVector::unpack(T& destination)
, the ByteVector automatically consumes bytes so the next unpack starts where the previous one left off. Then the user doesn't have to manually calculate the offsets, they just have to unpack to the appropriate destination in sequence.Client
not needing to know about the firmware variantprint
method defined in msp::msg::Message
meant that I could do a single method definition std::ostream& operator<<(std::ostream& s, const msp::Message& val) { return val.print(s); };
and any message with a virtual print override could be printed. If the message didn't have its own definition, the parent method would be called with no apparent side effects.print
function and you do not get output on the screen, you do not know if it is because of the missing implementation or because you simply did not receive any messages of that type.Client
library to have the MSPv2 code paths. Do you want to check out the MSPv2_support branch from my fork? That has everything, and I can make adjustments there before doing pulls into master.How large is your MSP2 feature branch? If it helps you, you could create a new MSP2 pull request that adds MSP2 support and everything that needs to be changed to enable MSP2 (e.g. including this PR).
It's just that it becomes difficult to review the PR if it adds too many changes.
Its only 25ish files including examples, about like master is now. There is a lot of resemblance to the master code base, but its all been changed in subtle but pervasive ways.
You can give it a try. If it's too much, I will continue with this PR.
ok so new PR against my full MSPv2 changes?
Yes. Can you create a new PR (and leave this open for now) that includes all the MSP2 support changes? I.e. the features that you originally wanted to merge, where I initially proposed to split these changes into multiple PRs. Let's try to do all of these changes in a single PR.
Superseded by https://github.com/christianrauch/msp/pull/28.
This has most of my internal changes to support MSPv2. Converted all messages to inherit from msp::msg::Message class. Turned ByteVector into an actual class. Added a new value type to support optional decoding of message fields. No changes to
MSP
,Client
, andFlightController
APIs.