Implement a better buffer-reading abstraction

[bzbarsky-apple]

Problem

We have a lot of places where we have a buffer (uint8_t*) and size and want to execute the following logic:

1. Make sure we have enough data in the buffer for the read we're about to do.
2. Do the read, with given endianness.
3. Advance the read position.
4. Decrement the amount of data we think is left in the buffer.

Various places get this wrong (e.g. by forgetting step 4), and it's a bit error-prone to force consumers to think about this set of steps all the time.

Proposed Solution

Implement a class that encapsulates the "buffer and length" state and the corresponding logic. PacketBuffer sort of has similar logic already, in that you can update the start position, but not everything we are dealing with in this code is necessarily a PacketBuffer.

We wanted something like this in #2168 anyway.

There are three main approaches I've thought of so far here:

1. The new class (call it BufferReader) exposes APIs that do all the above and call various *Endian::Read* functions under the hood. So it might have a function called Read16LE or Read32BE.
2. Have separate LittleEndianReader and BigEndianReader classes. This avoids the LE/BE suffixing, at the cost of not allowing mixed-endianness reads from the same buffer. In practice, I don't expect this to be an issue: right now there is no big-endian reading going on in CHIP, much less mixed-endian reading.
3. Modify the *Endian::Read* functions to take instances of some BoundedBuffer class and operate on them. These could either be in addition to the existing functions or replacing them. Some confusion possible here with BufBound, but maybe we can figure out better naming.

From the point of view of minimizing footguns, it seems like option 3 and "replacing" is the way to go. It has the drawback of forcing all reads through a fallible (if there is not enough buffer left) API.

From the point of view of having composable simple pieces, I think option 2 is probably the right one. We would then need to go through our existing Read* calls and replace them as it makes sense, and keep an eye on what option people use in new code.

@andy31415 @rwalker-apple @mspang @bhaskar-apple @shana-apple @BroderickCarlin any preferences amongst the options here?

[issue-label-bot]

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.99. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[bzbarsky-apple]

Oh, and I guess all the approaches here have the main drawback of doing length checks on every read instead of one up-front length check and then a bunch of reads. I think that the latter is much more error-prone (e.g. due to people adding a read without updating the length check, or conditional reads or whatnot), and that the codesize/performance hit here is pretty minimal and likely to be worth it.

[BroderickCarlin]

I believe I had brought this up in the past but ended up stepping back as it seemed we had a solution in place but SmartThings has a generic buffer management C module we would potentially be willing to open source for use in CHIP if it's deemed of interest. In short, our implementation is very closely aligned with #1 above with a generic buf_t struct that owns a byte array, the length of said buffer, and a flag indicating any errors. A slew of utility functions can then act on said struct to read/write any specified standard type in any specified endianness and check if there are any error flags set. As you point out, this approach is not optimized for performance but we have not found this to be an issue

[bzbarsky-apple]

I think one important question there is whether we want a C++-style API for this (which would then need to be built on top of the module SmartThings has). If not, I think taking the module might make sense. If we do, it might be simpler to just write a thing per one of my options above: it would be less code because it could use the existing CHIP and nl facilities for endian-specific reading.