Packet builder #209

Y-Less · 2023-03-10T09:35:50Z

Another draft PR for comments, since it is a larger one. I've basically written a class to do all the maths and position tracking for building up packets with lots of compressed data in.

The current code has lots of instances of code to build packets like:

data[0] = EXTENDED_CONNECTION_ABORT_MULTIPLEXOR;
data[1] = static_cast<std::uint8_t>(reason);
data[2] = 0xFF;
data[3] = 0xFF;
data[4] = 0xFF;
data[5] = static_cast<std::uint8_t>(pgn & 0xFF);
data[6] = static_cast<std::uint8_t>((pgn >> 8) & 0xFF);
data[7] = static_cast<std::uint8_t>((pgn >> 16) & 0xFF);

Or deconstruct packets like:

auto &data = message->get_data();
parentInterface->languageCommandTimestamp_ms = SystemTiming::get_timestamp_ms();
parentInterface->languageCode.clear();
parentInterface->languageCode.push_back(static_cast<char>(data.at(0)));
parentInterface->languageCode.push_back(static_cast<char>(data.at(1)));
parentInterface->timeFormat = static_cast<TimeFormats>((data.at(2) >> 4) & 0x03);
parentInterface->decimalSymbol = static_cast<DecimalSymbols>((data.at(2) >> 6) & 0x03);
parentInterface->dateFormat = static_cast<DateFormats>(data.at(3));
parentInterface->massUnitSystem = static_cast<MassUnits>(data.at(4) & 0x03);
parentInterface->volumeUnitSystem = static_cast<VolumeUnits>((data.at(4) >> 2) & 0x03);
parentInterface->areaUnitSystem = static_cast<AreaUnits>((data.at(4) >> 4) & 0x03);
parentInterface->distanceUnitSystem = static_cast<DistanceUnits>((data.at(4) >> 6) & 0x03);
parentInterface->genericUnitSystem = static_cast<UnitSystem>(data.at(5) & 0x03);
parentInterface->forceUnitSystem = static_cast<ForceUnits>((data.at(5) >> 2) & 0x03);
parentInterface->pressureUnitSystem = static_cast<PressureUnits>((data.at(5) >> 4) & 0x03);
parentInterface->temperatureUnitSystem = static_cast<TemperatureUnits>((data.at(5) >> 6) & 0x03);

This is (IMHO) very complex to read and error-prone, though admittedly it is very explicit in what is happening and where things are being stored.

Anyway, those two examples rewritten with this PR would look something like:

isobus::ParameterGroupBuilder builder {};
builder.write<std::uint8_t>(EXTENDED_CONNECTION_ABORT_MULTIPLEXOR);
builder.write(static_cast<std::uint8_t>(reason));
builder.pad(24);
builder.write(pgn, 24);

isobus::ParameterGroupBuilder builder(message->get_data());
parentInterface->languageCommandTimestamp_ms = SystemTiming::get_timestamp_ms();
builder.read((char *)&parentInterface->languageCode, 16);
builder.skip(4);
builder.read(parentInterface->timeFormat, 2);
builder.read(parentInterface->decimalSymbol, 2);
builder.read(parentInterface->dateFormat);
builder.read(parentInterface->massUnitSystem, 2);
builder.read(parentInterface->volumeUnitSystem, 2);
builder.read(parentInterface->areaUnitSystem, 2);
builder.read(parentInterface->distanceUnitSystem, 2);
builder.read(parentInterface->genericUnitSystem, 2);
builder.read(parentInterface->forceUnitSystem, 2);
builder.read(parentInterface->pressureUnitSystem, 2);
builder.read(parentInterface->temperatureUnitSystem, 2);

Might just need a tiny bit of tweaking for types/widths on some of the variables, but otherwise that's complete.

Now I've already mixed up pad (for writing) and skip (for reading) in my own code, so it might make more sense to have the reading and writing capabilities split in to two different classes, but the core algorithms are there. Interesting, one thing I noticed is that the spec goes in to some detail about how odd-bit-width data spans byte boundaries, and this code correctly implements that, but then all the PGNs in the spec use padding bits to explicitly avoid those cases. And thus this code also checks for those common cases where data is byte-aligned or fully contained within a byte to better optimise for those cases.

Y-Less · 2023-03-10T09:38:34Z

I don't know where the best place for this is. I've not committed any tests (but did run some and am using it locally). This is mostly just the initial basic algorithm. And even within that I'm sure some parts could be compressed - there are a few bits of repeated code, especially in the sections handing large data spanning multiple bytes, but honestly I don't think rearranging the code is worth it to save a few lines at the expense of complexity.

Y-Less · 2023-03-10T09:39:30Z

Two big formatting things I've noticed that probably need changing - I've used #pragma once instead of include guards, and early returns, neither of which seem tot be used anywhere else in the codebase.

ad3154 · 2023-03-12T17:49:49Z

I'll take a look at it and make some comments. Something like this has been requested before, so it could be helpful for people who do not want to do manual shifting and masking. The style is fairly off from the rest of the repo, so a general comment would be to try and match the rest of the repo's style if possible, I'll try and comment some examples.

To answer some of your questions up-front, for #pragma generally we lean on the Autosar rule:

Since it's compiler specific behavior and is not always even implemented, it should be avoided for a highly cross-platform library like ours.

As for multiple return paths, I like to lean on MISRA's rule:

This is a fairly controversial rule in C++ I will admit, as there's nothing "wrong" with early returns. Most of the benefit though is that code will always have a normal code flow top to bottom, there's only 1 place where you need to put a breakpoint to catch the return value of the function (makes debugging easier I would argue, especially on devices that have a maximum number of breakpoints (!!) like the stm32f4 which can only have something like 7 breakpoints), and lastly it helps ensure allocated resources get cleaned up, since you'd either need to copy the lines of code that do that for each return statement, or do it once at the bottom (though, a pure RAII approach mitigates this). The main downside to a single return path as I see it is that there is additional indenting in the code due to making those early returns into more highly nested conditionals, but I have rarely found that a compelling reason to lose the benefits while debugging. I don't think I'd prevent a merge due to an early return, but I don't prefer them, especially in a library marketed towards an industry that will skew towards that Autosar/Misra side of things.

ad3154

I've made a first pass through it. Generally I think the library could use something that is similar to this, but it seems like a number of your code's assumptions have made their way into here, and I have concerns about it not working correctly depending on a machine's endianness. It also isn't quite generic enough to fit a library use-case, up to the ETP protocol size limit.

So, I guess where we go from there is kind of up to you. This kind of thing is an ideal candidate for heavy unit testing since it's easy to provide lots of stateless test cases, so if you want to keep working on it and improving it, and if you can write unit tests for it, I think we could revisit adding it in. Or, if you think it's meeting your needs in your software, you are of course welcome to keep using it in your fork.

It can be tough to convey emotion through a GitHub PR review, so I just wanted to say that I do appreciate all your PR submissions, including this one. I probably spent at least a good hour doing this review (it takes time to read the code to come up-to-speed on what it's doing), which I wouldn't do if I didn't think it was valuable.

ad3154 · 2023-03-12T17:50:58Z