Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a basic message reader for the data model. #3575

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple commented Nov 2, 2020

The idea is to expose a semantic API where consumers can read things
like "a cluster id" or "an endpoint id", without necessarily making
assumptions about the sizes/representations of these things.

The API does still expose facilities for reading raw unsigned integers
of specific sizes.

More types will be added to this API as needed.

Problem

We don't have a good setup for reading from a buffer in the data model without ending up with possible length overruns, etc.

Summary of Changes

Add a data model reader.

Fixes #2168

// methods our superclass has.
class DataModelReader : private Encoding::LittleEndian::Reader
{
typedef Encoding::LittleEndian::Reader Super;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Super sounds like an odd name. Maybe BaseReader ?

Is there a value in private derivation? I wonder if it would be easier to compose rather than derive, and then I would have:

...
private:
   Encoding::LittleEndian::Reader mReader;
public:
   uint16_t OctetsRead() const { return mReader.OctetsRead(); }
    ...

This looks just as compact memory wise and it seems to avoid the extra typedef.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can do the composition thing, good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that requires me to make the templated Read public on LittleEndian::Reader. I think I'm OK with that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call it 'RawRead' or something. May also be useful to directly write RawRead<uint8_t>(....) instead of just RawRead(...) to make if very explicit.

Or make this class conver ReadFoo into the correct Read16/Read32 and such. Tedious if someone decides to change sizes of some of our basic data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not "if", "when". We know people want to change the sizes of some of them; I am trying to land the groundwork for making it painless.... Hence avoiding the explicit typing of Read or the use of specific Read* functions.

return static_cast<uint16_t>(read_ptr - buffer);
// Skip first byte, because that's the always-0 frame control.
uint8_t ignored;
err = reader.ReadOctet(&ignored)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how readable this is! ❤️

Should we have a SkipBytes() call to avoid the ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ignored is an artifact of our encoding right now being something we just made up that has nothing to do with anything, really. In the long run we won't have a byte we're ignoring here.

But yes, if we end up with "real" code that needs to do that, we can add that.

@bzbarsky-apple bzbarsky-apple force-pushed the data-model-message-reader branch from 2b4202a to 08877f6 Compare November 2, 2020 19:10
@boring-cyborg boring-cyborg bot added the lib label Nov 2, 2020
@github-actions
Copy link

github-actions bot commented Nov 2, 2020

Size increase report for "nrfconnect-example-build" from c4ebfbe

File Section File VM
chip-lock.elf shell_root_cmds_sections -8 -8
chip-lock.elf text -40 -40
chip-lock.elf rodata -360 -360
chip-lighting.elf shell_root_cmds_sections 8 8
chip-lighting.elf text -40 -40
chip-lighting.elf rodata -360 -360
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,2828
.debug_str,0,373
.debug_abbrev,0,224
.debug_line,0,84
.debug_frame,0,12
shell_root_cmds_sections,-8,-8
text,-40,-40
.debug_ranges,0,-248
rodata,-360,-360
.debug_loc,0,-417

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

sections,vmsize,filesize
.debug_info,0,2828
.debug_str,0,373
.debug_abbrev,0,224
.debug_line,0,80
.debug_frame,0,12
shell_root_cmds_sections,8,8
text,-40,-40
.debug_ranges,0,-248
rodata,-360,-360
.debug_loc,0,-421

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

sections,vmsize,filesize


The idea is to expose a semantic API where consumers can read things
like "a cluster id" or "an endpoint id", without necessarily making
assumptions about the sizes/representations of these things.

The API does still expose facilities for reading raw unsigned integers
of specific sizes.

More types will be added to this API as needed.
@bzbarsky-apple bzbarsky-apple force-pushed the data-model-message-reader branch from 08877f6 to 4654e7a Compare November 3, 2020 03:08
@bzbarsky-apple
Copy link
Contributor Author

@saurabhst @BroderickCarlin @hawk248 @jelderton Please take a look?

@andy31415 andy31415 merged commit 84847dd into project-chip:master Nov 3, 2020
@bzbarsky-apple bzbarsky-apple deleted the data-model-message-reader branch November 3, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement better buffer abstractions in Silicon Labs code
5 participants