Let byte_streams use a more correct element type: uint8_t

[jputcu]

For some time I've been using my fifo type to encode/decode communication data. Recently I noticed there is a byte_stream_reader/writer which actually could replace this: less reinventing the wheel, more unit tests,...
However, unlike the default raw buffer type: uint8_t (or etl:byte) this uses char as its underlying type which I noticed when trying to construct it with a span<const uint8_t>.
Currently I'm using the data + size constructors but this feels like a bit unlucky.

Would it be sensible to fix this?

[mike919192]

Currently I'm using the data + size constructors but this feels like a bit unlucky.

For a little better workaround, you should be able to use the etl::span reinterpret_as function to convert uint8_t span to char span. The reinterpret operation should be fine because it is just converting between unsigned char to char.

[jputcu]

Thanks @mike919192 for bringing this method to my attention, I think I can use it in other places where are needed to that conversion. Unfortunately, I'm also in the process of converting to more standard C++, so moving etl::span to std::span, which doesn't have this method.

[mike919192]

I think I can use it in other places where are needed to that conversion

Be mindful that all the precautions regarding reinterpreting still apply. Misuse can lead to violating aliasing rules. Thats why I mentioned that this particular conversion is OK because converting to char is explicitly one of the OK conversions.

[tzijnge]

I think it would make sense to template byte_stream_reader and byte_stream_writer on the 'byte' type. That way it can interface with whatever the user regards a suitable representation of byte, as long as sizeof equals 1. This includes signed char, unsigned char, char, int8_t, uint8_t, char8_t, std/etl::byte.
After templating on a byte type, the dangerous constructors from void* can be deprecated.

Slightly related: I think byte_stream_reader and byte_stream_writer could also have support for reading and writing etl/std::byte

Also slightly related: byte_stream_reader should be constructed from 'const' storage?

[jputcu]

I just got bitten by the fact it uses char underlying: byte_stream_writer, written uint8_t types, then std::accumulate(d.begin(), d.end(), uint16{}) to calculate the checksum. This gave incorrect results because all bytes are offered as singed char.

[mike919192]

I just got bitten by the fact it uses char underlying: byte_stream_writer, written uint8_t types, then std::accumulate(d.begin(), d.end(), uint16{}) to calculate the checksum. This gave incorrect results because all bytes are offered as singed char.

Maybe you can post an example of your error? Generally speaking there should not be a problem if the underlying storage is treated as char. One example that I can think of is std::ifstream and std::ofstream in binary mode treat arbitrary data as char.

[jputcu]

That's just it, I didn't get any warnings or errors, sometimes my checksum was ok, sometimes it was wrong.
Under the hood, std::accumulate was happily adding int8_t to uint16_t.
Binary data I added was converted to char, so larger values were converted to negative values, resulting in subtraction instead of additions.

[mike919192]

So in this statement std::accumulate(d.begin(), d.end(), uint16{}), is d your char array? That is what it sounds like from your description and that would mean that your error is not just because signed conversion but also because of misinterpreting the data type. Or another way to put it is using static_cast where you need a reinterpret_cast.

I think this unit test is relevant. The byte_stream_reader class helps with reinterpreting the data.

etl/test/test_byte_stream.cpp

Line 660 in 3e4d41c

TEST(write_read_uint16_t)

[jputcu]

Actually, I'm using byte_stream_writer to create binary data to be sent.
Something like:

std::array<uint8_t, 300> data_buffer;
etl::byte_stream_writer bs(data_buffer, etl::endian::little);

bs.write(uint8_t{0xa0}); // This broke my std::accumulate because this resulted in a substraction
...

// checksum calulation I tried but is incorrect
auto crc = std::accumulate(bs.cbegin(), bs.cend(), uint16_t{0});

// Actual correct
uint16_t crc{};
for ( const auto b : data ) {
   crc += static_cast<uint8_t>(b);
}

[mike919192]

OK I see the issue now and before I was misunderstanding. I am the same where I also prefer to use uint8_t for byte arrays, but lots of APIs (STL and ETL) use char (probably for historical reasons).

I don't think your problem is with the bs.write though. I think your problem is the call to accumulate because the bs iterators are using char type, where you want to treat the data as unsigned.

There's 2 ways I can think you can call accumulate correctly. The simpler would be to call it on the data_buffer directly. The other would be to use a delegate to perform the cast in the accumulate algorithm.

auto crc = std::accumulate(data_buffer.cbegin(), date_buffer.cend(), uint16_t{0});

or

auto crc = std::accumulate(bs.cbegin(), bs.cend(), uint16_t{0}, [](auto init, auto a){ return init + static_cast<uint8_t>(a); });