Closed ftab closed 3 years ago
Hi Dennis, Thanks for the warm words about my library. I'm happy when other developers find my work useful. Recently I saw someone online describing my library as "building a house out of matches". It may be true, but these "matches" allow implementation of various crazy / complex protocols with multiple levels of customization with little effort (once you get hold of how to use the library of course).
You did a great job in understanding how to use the library and what needs to be done. I'd say both of your approaches are feasible and more or less equal in complexity in terms of implementation effort and runtime speed performance. As for the final binary code size, I'm not sure, testing is needed. Unless you are implementing something bare-metal, it shouldn't bother you much.
Let's start with the "variant" approach. Note, that COMMS library is handles all the "length" fields as containing the remaining bytes after the length field itself (not including the latter). In your case we just need to add "4" when serializing the value of the field. The same goes for the "Length" field in the main message frame. As the result, your length field should be defined as:
using LengthField =
comms::field::IntValue<
MyFieldBase,
uint16_t,
comms::option::NumValueSerOffset<4>
>;
The ParameterIDField
looks good, but I'd use ValidNumValue option instead of ValidNumValueRange to avoid repetition of the parameter
template <int Num>
using ParameterIDField =
comms::field::IntValue<
MyFieldBase,
ParamId,
comms::option::DefaultNumValue<Num>,
comms::option::ValidNumValue<Num>,
comms::option::FailOnInvalid<>
>;
Now, the COMMS library allows you to implement your own read/refresh functionality to any field as long as you use HasCustomRead and HasCustomRefresh options to notify other components that these functions are not default.
template <int Num, typename T>
class Parameter " public
comms::field::Bundle<
MyFieldBase,
std::tuple<
ParameterIDField<Num>,
LengthField,
T
>,
comms::option::HasCustomRead,
comms::option::HasCustomRefresh
>
{
using Base = ...; // Repeated Base class definition, my be required for certain compilers
public:
// Provide convenience access functions to bundle members
COMMS_FIELD_MEMBERS_ACCESS(id, length, val);
// Custom read function
template <typename TIter>
comms::ErrorStatus read(TIter& iter, std::size_t len)
{
auto es = Base::template readUntil<FieldIdx_val>(iter, len); // read id and value
if (es != comms::ErrorStatus::Success) { return es; }
auto consumedLength = Base::template lengthUntil<FieldIdx_val>();
auto remainingLength = len - consumedLength;
auto expectedLength = field_length().value();
if (expectedLength < remainingLength) { /* report error, data is malformed */}
es = Base::template readFrom<FieldIdx_val>(iter, remainingLength);
if (es != comms::ErrorStatus::Success) { reutrn es; }
// consume remaining bytes as protection against malformed data
assert(field_value().length() <= remainingLength); // debug code verifying correction
auto lengthToConsume = remainingLength = field_val().length();
std::advance(iter, lengthToConsume);
return comms::ErrorStatus::Success;
}
// Custom refresh function (to bring field to a consistent state before writing)
bool refresh()
{
auto expectedLength = field_val().length();
auto recordedLength = field_length().value();
if (recordedLength == expectedLength) { return false; } // state not changed
field_length().value() = expectedLength;
return true; // state changed
}
};
The empty value parameter I'd define using comms::field::NoValue:
template <int Num>
using EmptyParameter = Parameter<Num, comms::field::NoValue<MyFieldBase>>;
Then your definition of the variant becomes:
class Message1Variant : public
comms::field::Variant<
MyFieldBase,
std::tuple<
Parameter<Message1ParameterIDs::X, UInt16Field>,
Parameter<Message1ParameterIDs::Y, UInt16Field>,
EmptyParameter<Message1ParameterIDs::Button>
>
>;
{
using Base = ... // redefinition of the base class, my be required by some compilers
public:
// Provide convenience access functions
COMMS_VARIANT_MEMBERS_ACCESS(X, Y, Button);
};
Now you define your message contents as list of variants
struct Message1Fields
{
using PropertiesList =
comms::field::ArrayList<MyFieldBase, Message1Variant>;
using All = std::tuple<PropertiesList>;
};
template <typename TBase>
class Message1
: public comms::MessageBase<TBase,
comms::option::StaticNumIdImpl<MsgId_Message1>,
comms::option::FieldsImpl<Message1Fields::All>,
comms::option::MsgType<Message1<TBase>>,
comms::option::HasName>
{
public:
COMMS_MSG_FIELDS_ACCESS(list); // there is only one "list" field.
};
The access to the parameters in the handling function or before sending may be performed like this:
auto& listField = msg1.field_list(); // access to the list field
auto& variantsVec = listField.value(); // access to the vector containing the variant fields
for (auto& varField : variantsVec) { // access to each variant field
... // process each field here
}
How to process each variant contents is up to you to decide. Please open documentation of comms::field::Variant and read through available functions. One of the approaches is to define your parameter handling class and use "currentFieldExec()" member function for dispatch
struct MyHandler
{
template <std::size_t TIdx, typename TField>
void operator()(TField& field)
{
... // TField is one of the types in the tuple of the variant ("Parameter")
... // switch on ID and store the value somewhere to process later
}
}
Another way to implement handler is provide "operator()" for every possible parameter
struct MyHandler
{
template <std::size_t TIdx>
void operator()(Parameter<Message1ParameterIDs::X, UInt16Field>& f)
{
auto xValue = f.field_val().value();
... // Store value of X somewhere
}
template <std::size_t TIdx>
void operator()(Parameter<Message1ParameterIDs::Y, UInt16Field>& f)
{
auto yValue = f.field_val().value();
... // Store value of Y somewhere
}
template <std::size_t TIdx>
void operator()(EmptyParameter<Message1ParameterIDs::Button>& field)
{
... // Store indication that button has been pressed
}
}
I'd have some kind of State struct containing values for for all the possible parameters wrapped in boost::optional or std::optional to have an indication whether it was actually received. The MyHandler object in its handling operators may update the state object. Then the message handling may proceed in analyzing the received values.
If you want to send such a message you need to prepare it properly:
Message1<MyMessageInterface> msg1;
auto& listField = msg1.field_list(); // access to the list field
auto& variantsVec = listField.value(); // access to the vector containing the variant fields
variantsVec.resize(3); // Prepare 3 parameters
auto& xParam = variantsVec[0].initField_X();
xParam.field_val().value() = ... // X value
auto& yParam = variantsVec[1].initField_Y();
yParam.field_val().value() = ... // Y value
auto& buttonParam = variantsVec[2].initField_Button();
msg1.doRefresh(); // Required to assign appropriate values to all the "length" fields in the bundles
... // Send the message
The second approach as treating every parameter as second message may also work. I'll try write a second message explaining it later today.
As for the treating parameters as extra messages (within a main message), I think (but I may be mistaken) you'll end up with writing more code than with "variant" approach. The main idea would be to have fields of every message start with "ArrayList" of raw bytes, followed by all the possible parameters wrapped in comms::field::Optional, which also marked to be missing by default.
Something like:
struct Message1Fields
{
using RawData = comms::field::ArrayList<MyFieldBase, std::uint8>;
using ParamX =
comms::field::Optional<
comms::field::IntValue<MyFieldBase, std::uint16>,
comms::option::MissingByDefault
>;
...
using All = std::tuple<RawData, ParamX, ParamY, ParamButton>;
}
Then all the basic operations on the message (read/refresh/length) need to be custom ones:
class Message1<...>
{
public:
COMMS_MSG_FIELDS_ACCESS(Data, X, Y, Button);
template <typename TIter>
comms::ErrorStatus doRead(TIter& iter, std::size_t len)
{
auto es = Base::template readFieldsUntil<FieldIdx_X>(iter, len); // reads only Data
if (es != comms::ErrorStatus::Success) return es;
... // Treat Data field as a buffer containing multiple parameter definition "messages".
... // Read them and dispatch to a handler (described below).
return comms::ErrorStatus::Success;
}
std::size_t doLength() { return Base::template doLengthUntil<FieldIdx_X>(); }
template <typename TIter>
comms::ErrorStatus doWrite(TIter& iter, std::size_t len) const
{
... // Write only Data field.
}
bool refresh()
{
... // Write all non-missing optionals to Data field.
}
};
Handler for Message1 parameters may look like this:
class Message1Handler
{
Message1& m_msg;
public:
Message1Handler(Message1& msg) : m_msg(msg) {}
void handle(XParamMsg& paramMsg)
{
m_msg.field_X().setExists(); // Mark the field as "exists" to indicate reception
m_msg.field_X().field().value() = paramMsg.field_val().value();
}
void handle(YParamMsg& paramMsg)
{
m_msg.field_Y().setExists(); // Mark the field as "exists" to indicate reception
m_msg.field_Y().field().value() = paramMsg.field_val().value();
}
...
};
Then when handling the original Message1 analyze what optional fields are marked as "exist" and act accordingly.
Hope it helps.
Also note that "hefty" part comes in defining your first message, then many classes you already defined are getting reused and implementing all other messages is getting significantly easier.
Thank you so much for your detailed reply! This helps a lot.
After considering those possibilities, I think I'd like to try the handler with a function for each possible parameter first, seems like the best so far in my limited knowledge (I recall reading something like a large switch being inefficient in your docs? some of these param lists could be 30+ and a large switch statement could get ugly and hard to maintain)
So, would that be something like this?
struct Message1ParameterState
{
std::optional<uint16_t> m_X{};
std::optional<uint16_t> m_Y{};
std::optional<bool> m_Button{};
};
struct Message1ParameterHandler
{
Message1ParameterHandler(Message1ParameterState &State) : m_State(State)
{
}
template <std::size_t TIdx>
void operator()(Parameter<Message1ParameterIDs::X, UInt16Field>& f)
{
auto xValue = f.field_val().value();
m_State.m_X = xValue;
}
template <std::size_t TIdx>
void operator()(Parameter<Message1ParameterIDs::Y, UInt16Field>& f)
{
auto yValue = f.field_val().value();
m_State.m_Y = yValue;
}
template <std::size_t TIdx>
void operator()(EmptyParameter<Message1ParameterIDs::Button>& f)
{
m_State.m_Button = true;
}
private:
Message1ParameterState& m_State;
};
class MessageHandler
{
public:
void handle(Message1<MyMessageInterface>& msg)
{
Message1ParameterState params;
Message1ParameterHandler paramHandler(params);
auto& listField = msg.field_list(); // access to the list field
auto& variantsVec = listField.value(); // access to the vector containing the variant fields
for (auto& varField : variantsVec) { // access to each variant field
varField.currentFieldExec(paramHandler);
}
/* do something with params.X, params.Y, params.Button */
}
void handle(MyMessageInterface& msg)
{
} // do nothing for all other messages
};
(As shown, the handler and state probably don't necessarily need to be separated, but I may end up having a large state object containing each of those structs instead)
Yes, it looks about right. Also you may notice, that initial part of handling every message looks very similar to all the messages. They all iterate over the list of the variant fields (different variants for every message though), dispatch them to their respective handlers before actually acting on the message. I would suggest considering templatizing this part and reuse it for every message.
Recently I also received some feedback saying that I may be mistaken about inefficiency of the "switch" statement. Modern compilers seem to generate direct dispatch tables (O(1) runtime complexity) or implement binary search for all the numeric "cases" (O(log(n)) runtime complexity) in case the values are too sparse.
The "currentFieldExec()" member function takes a stored index of the tuple element (runtime information) and casts its into appropriate type. Although it uses template functions to do the mapping, the generated code will be equivalent to having something like this:
if (idx < N/2) {
if (idx < N/4) {
...
else {
...
}
} else {
if (idx < 3/4N) {
...
}
else {
...
}
}
In other words its runtime complexity is always O(log(n)). If inefficiency of switch statement has became a myth, then you may end up having a better binary code when using it. Besides, you will end up implementing the same number of operator() functions as you would with "cases". If you do decide to go with "switch", then you should use generated "accessField_*()" member functions to cast it yourself to the appropriate type:
switch(varField.currentField()) {
case Message1Variant::FieldIdx_X:
xValue = varField.accessField_X().field_val().value();
break;
case Message1Variant::FieldIdx_Y:
yValue = varField.accessField_Y().field_val().value();
break;
...
};
P.S. If you encounter some compilation errors you cannot understand yourself, you are welcome to send me a direct email, instead of doing it in the ticket here.
Wow. So, I did some reading. A switch statement nowadays is either compiled to a jump table (O(1)) or a binary search tree (O(log N)) depending on how sparse the cases are. So, that probably means the jump table in the case of parameter IDs, and I guess BST for message IDs (unless I split message IDs into two u8s and thus two switches...?)
Guessing the performance difference will be negligible in all possible worst case scenarios either way (a few hundred messages in the first few seconds probably, depending on update rate...) - the computer will spend much more time twiddling its thumbs waiting for a message
I would be curious to know what it'd mean for message handling. How would comms library have to be modified in order to support a message dispatch that uses a switch statement? I guess it's possible to just do the switch under the generic catch-all in the current version?
Probably doesn't matter enough to spend much more thought on it. (Premature optimization, 99% of time is spent on 1% of the code/performance, etc.)
I just now got writing/reading my first couple of messages working, and I think the way you laid it out for me will work perfectly fine 😄
Hi Dennis, The compiler has much greater code introspection and generation capabilities, then libraries written in C++. Unfortunately I cannot generate "switch" statement and/or its "cases" out of list of types / values provided in template parameters. At least, I don't know how. But I can generate various dispatch tables, which I'm doing in the COMMS library.
For example, comms::MsgFactory (used by comms::protocol::MsgIdLayer) contains compile time logic which does basically the following:
The price for creation of these dispatch tables are usage of virtual functions and having multiple v-tables (which can be a problem for bare-metal devices with small ROM).
I've also considered creation of similar dispatch tables for previously mentioned "currentFieldExec()" for the variant fields to get O(1) runtime performance. Even if it's possible, I'm not sure I'm willing to pay the price for using dispatch indirection and the inevitable increase in the code size (due to having multiple v-tables for dispatch).
The bottom line, writing boilerplate code manually with "switch" statement will definitely give you a better runtime and code size performance. However, it increases number of places in your code you need to update once you decide to introduce a new message into your protocol, and slow down significantly the development time. When using the infrastructure provided by the COMMS library and adding the new message type to a tuple, support for it is automatically added to message creation infrastructure (in comms::MsgFactory and comms::protocol::MsgIdLayer) as well as handling (comms::GenericHandler)
Besides, I designed COMMS library not to enforce a particular way of doing things (the docs describe a recommended one). You can use what is provided and replace some functionalities / components with custom ones if default behaviour is not good enough for your needs.
I like the polymorphic dispatching anyway. Feels more Cplusplussy 😄
What would you do about parameter IDs that aren't recognized? Currently I get ErrorStatus::InvalidMsgData, but I think if a message is otherwise well-formed I should ignore parameters to which I don't know the ID.
I would create additional "UnknownParameter" that doesn't fail its read and put it last in the Variant's list of parameters. For example:
class UnknownParameter : public
comms::field::Bundle<
MyFieldBase,
std::tuple<
comms::field::IntValue<MyFieldBase, std::uint8>, // Any numeric ID
LengthField,
comms::field::ArrayList<MyFieldBase, std::uint8> // Just raw data
>,
comms::option::HasCustomRead,
comms::option::HasCustomRefresh
>
{
using Base = ...; // Repeated Base class definition, my be required for certain compilers
public:
// Provide convenience access functions to bundle members
COMMS_FIELD_MEMBERS_ACCESS(id, length, val);
// Custom read function
template <typename TIter>
comms::ErrorStatus read(TIter& iter, std::size_t len)
{
auto es = Base::template readUntil<FieldIdx_val>(iter, len); // read id and value
if (es != comms::ErrorStatus::Success) { return es; }
auto consumedLength = Base::template lengthUntil<FieldIdx_val>();
auto remainingLength = len - consumedLength;
auto expectedLength = field_length().value();
if (expectedLength < remainingLength) { /* report error, data is malformed */}
es = Base::template readFrom<FieldIdx_val>(iter, remainingLength);
assert(es == comms::ErrorStatus::Success); // ArrayList is expected to consume all the data without error
return es;
}
// Custom refresh function (to bring field to a consistent state before writing)
bool refresh()
{
auto expectedLength = field_val().length();
auto recordedLength = field_length().value();
if (recordedLength == expectedLength) { return false; } // state not changed
field_length().value() = expectedLength;
return true; // state changed
}
};
Then your variant becomes:
class Message1Variant : public
comms::field::Variant<
MyFieldBase,
std::tuple<
Parameter<Message1ParameterIDs::X, UInt16Field>,
Parameter<Message1ParameterIDs::Y, UInt16Field>,
EmptyParameter<Message1ParameterIDs::Button>,
UnknownParameter
>
>;
{
using Base = ... // redefinition of the base class, my be required by some compilers
public:
// Provide convenience access functions
COMMS_VARIANT_MEMBERS_ACCESS(X, Y, Button, Unknown);
};
NOTE: that default behaviour of "read" operation of comms::field::Variant class is to attempt "read" of every type in the order of their definition. The read operation stops when the success is reported (or the list is exhausted and then error you mentioned is reported). It may be quite inefficient for variants that may have long lists of supported types. To improve the performance (only if needed) you may try to define your own "read()" function, just like you did with Bundle (Parameter above), and put the "switch"statement inside if it significantly improves things for you.
Hope it helps.
Also note, that ArrayList uses std::vector to hold its data and will copy the relevant bytes from input buffer there. If you don't use any type of circular buffer and your input bytes are always sequential, they you may want to consider passing "comms::option::OrigDataView" option to the definition of the ArrayList. It will use comms::util::ArrayView as internal data storage (instead of std::vector). The implications are that such field cannot outlive the input buffer (which is usually true in most cases, unless you store message object itself for future access).
Thanks! That ought to do it. Does it necessarily need to have an ArrayList defined as the value field to consume the bytes, or could it just have a NoValue and then the custom read simply advances the iterator by the value of the length field?
Nice ArrayView - looks suspiciously like gsl::span which I use a lot! Does it make more sense to use that for all of my uses of ArrayList? The incoming data is already in a QByteArray that's been passed to me via a signal from a reading thread, and the buffer is freed only after parsing of the message is complete. I don't see myself keeping the message object itself around, only maybe the functors / handler objects which would copy data in only as needed.
NOTE: that default behaviour of "read" operation of comms::field::Variant class is to attempt "read" of every type in the order of their definition. The read operation stops when the success is reported (or the list is exhausted and then error you mentioned is reported). It may be quite inefficient for variants that may have long lists of supported types. To improve the performance (only if needed) you may try to define your own "read()" function, just like you did with Bundle (Parameter above), and put the "switch"statement inside if it significantly improves things for you.
I noticed this as I was putting in some sample handmade data with which to test. Indeed it could get hairy (30+ parameters in some cases), especially in the worst case scenario of the parameters coming in in reverse order to how they're defined (edit: what I said don't make no sense). However, at the rate I'm likely to be exchanging messages I think the performance hit would be negligible except for maybe on the very weakest of the possible target devices (maybe non-negligible on a Pi Zero underclocked to 500 MHz 😜). I'll do some measurements.
Yes, NoValue field should do the job as well. There is also the comms::option::EmptySerialization option that can be passed to any field type, which will have the same outcome.
comms::field::ArrayList is a wrapper around some storage type (std::vector of fields or raw data by default), which provides proper API used by other components. You should not replace provided comms::field::ArrayList field, but you can change its default storage type. Usage of "comms::option::OrigDataView" will make it "comms::util::ArrayView". You can also use comms;:option::CustomStorageType option and provide any type you want as long as it provides same public API as std::vector or "comms::util::ArrayView".
Here's what I've got so far:
template <int Num, typename T>
using ParameterBase =
comms::field::Bundle<MyFieldBase,
std::tuple<ParameterIDField<Num>, ParameterLengthField, T>,
comms::option::HasCustomRead,
comms::option::HasCustomRefresh>;
template <int Num, typename T> class Parameter : public ParameterBase<Num, T>
{
using Base = ParameterBase<Num, T>; // required for certain compilers
public:
// Provide convenience access functions to bundle members
COMMS_FIELD_MEMBERS_ACCESS(id, length, val);
// Custom read function
template <typename TIter>
comms::ErrorStatus read(TIter& iter, std::size_t len)
{
// gsl::span<const char> dataSpan{iter, gsl::narrow<ptrdiff_t>(len)};
// debugDumpChar(Debug::Level::Verbose, dataSpan);
auto es = Base::template readUntil<FieldIdx_val>(
iter, len); // read id and length
if (es != comms::ErrorStatus::Success)
{
return es;
}
auto consumedLength = Base::template lengthUntil<FieldIdx_val>();
auto remainingLength = len - consumedLength;
auto expectedLength = field_length().value();
if (expectedLength > remainingLength)
{
/* the parameter is claiming it is bigger than the buffer that
* remains, this is definitely a malformed parameter */
debugPrint(Debug::Level::Warning,
std::string("Malformed data, ") + Fmt::u(len) +
" len, " + Fmt::u(consumedLength) + " consumed, " +
Fmt::u(remainingLength) + " remaining, " +
Fmt::u(expectedLength) + " expected");
// debugDumpChar(Debug::Level::Warning, dataSpan);
return comms::ErrorStatus::NotEnoughData;
}
/* else, the buffer that remains can be larger because it contains more
* parameters - do not greedily consume the buffer, assume the param
* is well-mannered (what could possibl-eye go wrong?) */
es = Base::template readFrom<FieldIdx_val>(iter, expectedLength);
return es;
}
// Custom refresh function (to bring field to a consistent state before writing)
bool refresh()
{
auto expectedLength = field_val().length();
auto recordedLength = field_length().value();
/* In case this is a parameter group within a parameter, refresh the group */
auto updated = field_val().refresh();
if (recordedLength != expectedLength)
{
field_length().value() = expectedLength;
updated = true;
}
return updated;
}
};
Note that I changed the comparison from < to > and removed the consumption of the rest of the buffer (parameter length field dictates how much to read, assuming it's not a corrupted packet--a layer external to me is providing checksum/packet integrity). ArrayList parameters by default consumed the entire remaining length as part of the array, where in this protocol they could instead be in the middle
Here's what I first tried for UnknownParameter
using UnknownParameterBase = comms::field::Bundle<
MyFieldBase,
std::tuple<UInt16Field, ParameterLengthField, comms::field::NoValue<MyFieldBase>>,
comms::option::HasCustomRead>;
class UnknownParameter : public UnknownParameterBase
{
using Base = UnknownParameterBase; // required for certain compilers
public:
// Provide convenience access functions to bundle members
COMMS_FIELD_MEMBERS_ACCESS(id, length, val);
// Custom read function
template <typename TIter>
comms::ErrorStatus read(TIter& iter, std::size_t len)
{
debugPrint(Debug::Level::Req, "Unknown data");
gsl::span<const char> dataSpan{iter, gsl::narrow<ptrdiff_t>(len)};
debugDumpChar(Debug::Level::Req, dataSpan);
auto es = Base::template readUntil<FieldIdx_val>(
iter, len); // read id and length
if (es != comms::ErrorStatus::Success)
{
debugPrint(Debug::Level::Req, "O_o");
return es;
}
auto consumedLength = Base::template lengthUntil<FieldIdx_val>();
auto remainingLength = len - consumedLength;
auto expectedLength = field_length().value();
if (expectedLength > remainingLength)
{
/* the parameter is claiming it is bigger than the buffer that
* remains, this is definitely a malformed parameter */
debugPrint(Debug::Level::Req,
std::string("Unknown parameter appears malformed, ") +
Fmt::u(len) + " len, " + Fmt::u(consumedLength) +
" consumed, " + Fmt::u(remainingLength) +
" remaining, " + Fmt::u(expectedLength) + " expected");
// debugDumpChar(Debug::Level::Req, dataSpan);
return comms::ErrorStatus::NotEnoughData;
}
std::advance(iter, expectedLength);
return comms::ErrorStatus::Success;
}
};
with my generic ParameterHandler containing the following function:
template <std::size_t TIdx> void operator()(UnknownParameter& f)
{
/* Ignore but warn */
debugPrint(Debug::Level::Warning,
std::string("Unknown parameter ID 0x") +
Fmt::x(f.field_id().value(), 4, true) + " ignored; skipping " +
Fmt::u(f.field_length().value()) + " bytes");
}
On the first test, with the NoValue and the std::advance (unless I'm doing it wrong) I get an unexpected result feeding it a command with an otherwise well-formed structure.
Input data:
0xBE, 0xEF, // SOM
0x12, 0x34, // ID
0x00, 0x0C, // len
0x00, 0x04, // param ID 4 = ?? I only know parameters 0, 1, and 3
0x00, 0x06, // param len
0x03, 0x00 // made up number for a made up parameter
Result of UnknownParameter read: (after scanning through parameters 0, 1, and 3 read failing)
Unknown data
Dumping 6 bytes of data
00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
----------------------------------------------------------------------
0000 00 06 00 01 03 00 ......
Unknown data
Dumping 2 bytes of data
00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
----------------------------------------------------------------------
0000 00 00 ..
O_o
ProtocolError
Seems as though it was called, and then advanced the read pointer but then assumed there were still 2 bytes left to read. This feels like an overflow of some sort to me. Advancing too far? Not enough? From the wrong offset? Or because the NoValue::length() is always 0 any iterator advancement within the custom read function is ignored? Not sure where the two 0x00 bytes came from.
If I change it to
using UnknownParameterBase = comms::field::Bundle<
MyFieldBase,
std::tuple<UInt16Field,
ParameterLengthField,
comms::field::ArrayList<MyFieldBase, uint8_t, comms::option::OrigDataView>>,
comms::option::HasCustomRead>;
and the end of read() to
/* Just consume the buffer--nothing will use it */
return Base::template readFrom<FieldIdx_val>(iter, expectedLength);
I get the expected result:
Unknown parameter ID 0x0004 ignored; skipping 2 bytes
followed by successful message dispatch
So, that should work, seems I do after all need the ArrayList for UnknownParameter.
Thank you so much for all of your help! I love the flexibility in this :) it's teaching me so much about C++
You are welcome. I'm still confused why it didn't work for you with NoValue field, the code looks fine to me. I would recommend spending a bit of extra time to understand why (whith prints of remaining data before and after call to advance), just to be sure there is no hidden bug somewhere in the library or your written code that can cause all sorts of troubles later.
Also based on experience, I'd say it's a good thing to have unused space consumption after read operation (event if it looks redundant). Quite often protocols get updated and sometimes developers think it is a good idea to add extra field or two at the end of the message and/or bundle of fields while updating length information accordingly. Having unused space consumption will keep you protocol implementation forward-compatible.
This thread is mostly about <variant> field. The tutorial4 in the official tutorials set covers the subject in more details.
Thanks for such a great system, I've been looking at it for quite a while and I felt really inspired once I started catching on 😄 I'm trying to implement a protocol which defines its basic stack like so:
I think I have the basic protocol stack layer properly built to handle this (SyncPrefix -> MsgId -> Size -> Payload) but then the parameter value can be any one of several data types. The primitive ones I think I've got down pat (basic ints, a string, etc). But parameters can come/go in any order and be any one of those data types or a group of them (i.e. the bottom right section of that table repeated all over again for the Value field). I've created Bundle fields for each specific pairing of param/length/data type, but I don't think there is a provision for a bundled field to have its own length, unlike ArrayList (if I understood ArrayList correctly). I saw Variant fields, but unless I'm wrong, for each message with, say, 3 parameters is going to look like this:
My other thought was just run the entire message payload layer through another protocol stack definition (MsgId -> Size -> Payload) and so on and so forth for any subgroups of parameters within, but 1) that'd be several new protocol stacks for each message I think?, 2) I don't see an example of how to do that, 3) not sure where to even begin to try to wrap my head around fitting that in (parameter dispatch?) during message dispatch
am I trying too hard to look for something to avoid boilerplate code in every message? The example I wrote above already looks pretty hefty just for a single message, and I've got...oh, about 100 messages to implement 😬
Thoughts?