refactor: properly handle network byte order in IntegerOption

[JKRhb]

I've got the impression that the handling of endianness in the IntegerOptions could already be fine by now. Therefore, I wanted to propose removing the two TODOs within the IntegerOption class. I am not entirely sure about big endian platforms, though. However, doing some manual testing did not reveal a problem so far.

Do you think the current state of this kind of option is fine, @JosefWN? Or do you think we should add some more formal testing in this regard?

[JKRhb]

Having another look at the IntegerOption methods, I now found a way to simplify the implementation while respecting both little and big endian platforms :) The PR should now be ready to be merged.

[JosefWN]

It looks a lot better!

What puzzled me a bit about the old code was the fairly extensive use of Endian.host. Regardless of what Endian.host is, we need to always read and write CoAP messages in Endian.big as per the spec.

Here for example, we read a CoAP (Endian.big) message as if it were Endian.host which is Endian.little on modern ARM/x86-64:

return ByteData.view(byteValue.buffer).getUint16(0, Endian.host);

So we have read the whole integer option "backwards", and need to reverse it at a later point, unless I misunderstood the old code which this PR fixes.

Because endianness can be a bit confusing, this is one of those places where (contrary to the linter), I would be explicit about the byte ordering, despite the default being big endian. In other words, I would specify endian: Endian.big or equivalent when reading data from and writing data to the UDP socket directly or indirectly via buffers etc.

For one, it clearly shows the developer which byte order is used (since we are manipulating things on a byte-level), which helps developers that are not experienced in network programming... and forgetful people like me.

For another, it's very easy to assume that Endian.big is always the default, but it's not from what I can see: ReadBuffer.getUint32 defaults to Endian.host: https://api.flutter.dev/flutter/foundation/ReadBuffer-class.html ByteData.getUint32 defaults to Endian.big: https://api.flutter.dev/flutter/dart-typed_data/ByteData/getUint32.html

It's easy to slip up, and debugging these issues can be a bit of a headache. Speaking of which, I wonder if we could spin up an emulated big endian environment using something like QEMU, just to see how it behaves on a little endian system. Will have a look in the coming days :)

[JKRhb]

It looks a lot better!

🎉

What puzzled me a bit about the old code was the fairly extensive use of Endian.host. Regardless of what Endian.host is, we need to always read and write CoAP messages in Endian.big as per the spec.

Yeah, I personally don't know why I didn't come up with the changes in this PR initially – I suppose there was some .reversed getter call left somewhere which caused some confusion 😅 I think the new API should now be a lot cleaner, as it originally promised :)

Because endianness can be a bit confusing, this is one of those places where (contrary to the linter), I would be explicit about the byte ordering, despite the default being big endian. In other words, I would specify endian: Endian.big or equivalent when reading data from and writing data to the UDP socket directly or indirectly via buffers etc.

For one, it clearly shows the developer which byte order is used (since we are manipulating things on a byte-level), which helps developers that are not experienced in network programming... and forgetful people like me.

That is a good idea, I updated the PR in the latest commit with a new constant and some documentation which should make the implementation a bit clearer :) Maybe another reference to RFC 7252 could also be added, since I also found it a bit confusing in the beginning that integers are the only type of option where the byte order needs to be respected.

Speaking of which, I wonder if we could spin up an emulated big endian environment using something like QEMU, just to see how it behaves on a little endian system. Will have a look in the coming days :)

That sounds awesome! :)

[JKRhb]

I added some more documentation – if the state of the PR is fine for you, @JosefWN and @shamblett, it could be merged :)

[JosefWN]

Dart's supported architectures: x64, IA32, ARM64, ARM, RISC-V (RV64GC).

As for testing CoAP on big endian, MIPS is primarily big endian but it's no longer supported by Dart. Even though most of the supported architectures are bi-endian, they are pretty much exclusively run in little endian mode, and it seems non-trivial to get something working with QEMU. We can test sometime in the future if we remember or if we get complaints, for now we've at least taken endianness into account and documented it :)

This PR LGTM!