Change to using a builder pattern for buffer reader.

[bzbarsky-apple]

For callers that read multiple things in a row with the reads not
being conditional on what was already read, this leads to slightly
smaller code. And in general it can lead to more readable code.

Problem

Want to use a builder pattern for our buffer reader.

Summary of Changes

Change the API to support this pattern.

Fixes #3495

[andy31415]

do these still need CHECK_RETURN_VALUE?

Should that be needed for StatusCode instead?

[bzbarsky-apple]

These do still need CHECK_RETURN_VALUE, so people don't forget to call StatusCode. Adding it to StatusCode is a good idea; I will do that.

[andy31415]

I noticed that our size actually increased. How about we place these in the header to allow for inlining instead? Does that make any difference in binary size?

[bzbarsky-apple]

The size increased because we only have a few callsites of each function so far. With inlining, we pay N bytes per callsite. With them out of line, we pay 4N + M * (# of callsites) bytes, where M is the cost of the function call; M < N.

I did measurements after adding some more callsites (as part of the fix for #2168) and at that point the out-of-line version is smaller than the inline one.

[andy31415]

why don't we read the vendor id directly? is it the wrong size/type? should we instead make the type match?

[bzbarsky-apple]

mVendorId is an Optional<uint16_t>. So yes,it's the wrong type for reading directly, but it's the right type for what we are trying to express.

We could address this by adding a method on Optional that sets it into the "have value" state and returns a reference to the value to be filled in. Then we could write something like:

  err = reader.Read16(&mVendorId.SetValue()).StatusCode();

or something. I wasn't sure whether it was desirable to add such an API to Optional; I've seen it on Optional-like things in the past, but there are safety tradeoffs with having an API that temporarily leaves the object in an invalid state...

[andy31415]

Ah, forgot about optional. Existing code makes sense then.

[github-actions]

Size increase report for "nrfconnect-example-build" from 76e0d7b

File	Section	File	VM
chip-lighting.elf	text	56	56
chip-lighting.elf	shell_root_cmds_sections	-8	-8
chip-lock.elf	text	60	60
chip-lock.elf	shell_root_cmds_sections	4	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,7774
.debug_line,0,1954
.debug_abbrev,0,1296
.strtab,0,121
.symtab,0,80
text,56,56
.debug_frame,0,48
.debug_aranges,0,40
.shstrtab,0,3
shell_root_cmds_sections,-8,-8
.debug_str,0,-45
.debug_ranges,0,-320
.debug_loc,0,-819

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,7774
.debug_line,0,1954
.debug_abbrev,0,1296
.strtab,0,121
.symtab,0,80
text,60,60
.debug_frame,0,48
.debug_aranges,0,40
shell_root_cmds_sections,4,4
.shstrtab,0,-1
.debug_str,0,-45
.debug_ranges,0,-320
.debug_loc,0,-827

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

[github-actions]

Size increase report for "esp32-example-build" from 76e0d7b

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	72	72

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_info,0,6939
.debug_line,0,2096
.debug_abbrev,0,1126
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIhEEvPT_,0,260
.shstrtab,0,239
.strtab,0,121
.symtab,0,112
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIyEEvPT_,0,101
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIjEEvPT_,0,100
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadItEEvPT_,0,100
.flash.text,72,72
.debug_frame,0,64
.debug_aranges,0,40
.xt.prop._ZN4chip8Encoding12LittleEndian7Write32ERPhj,0,-1
.debug_str,0,-28
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIyEEiPT_,0,-88
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadItEEiPT_,0,-168
.debug_ranges,0,-176
.debug_loc,0,-353

[Damian-Nordic]

BufBound which handles the opposite functionality has a bit different interface. For instance, it can write both little-endian and big-endian values to a single buffer. Do you think it would be worth using similar approach in BufferReader to keep the classes more aligned?

[bzbarsky-apple]

BufBound which handles the opposite functionality has a bit different interface. For instance, it can write both little-endian and big-endian values to a single buffer.

Yes, I considered that approach (see #2914 approach number 1), but in practice we only end up reading little-endian things in CHIP as far as I can tell, at least once you move above the IP packet layer. And we haven't had any use cases for mixed-endian reading from a single buffer so far. So I opted for having a simpler API for each read at the expense of a bit of flexibility we don't actually plan to use.

[bzbarsky-apple]

@saurabhst @jelderton @BroderickCarlin