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

Move Away from Bitstring Format Strings #57

Closed
medley56 opened this issue Aug 29, 2024 · 1 comment · Fixed by #63
Closed

Move Away from Bitstring Format Strings #57

medley56 opened this issue Aug 29, 2024 · 1 comment · Fixed by #63

Comments

@medley56
Copy link
Owner

In all of our data encoding classes (e.g. StringDataEncoding), we have a standard interface method called _get_format_string, which returns a bitstring-style format string like "bytes:5" (for 5 bytes) or "uint:12" (for a 12-bit unsigned int). These were very convenient when all of our parsing was based on the bitstring library but now that we are using Python bytes objects directly, we can be more efficient about how we communicate what to parse.

Our first idea is to pass a tuple with a type and a length. e.g. instead of "bytes:5", pass ("bytes", 5). This saves a string split step inside the PacketData.parse method, which would otherwise be called for every data item in every packet.

Note that StringDataEncoding._get_format_string actually returns an additional value for the number of bits to skip after parsing. This is to support the concept of a string that is terminated with a termination character or string such as \x00\x00\xff\xff, which shouldn't be part of the parsed value but instead should be skipped over after parsing. This position update (packet_data.pos += skip_bits_after) is actually handled inside of StringDataEncoding, so the interface to PacketData.parse is unchanged but its internal pos attribute is being modified from an external source.

@greglucas
Copy link
Collaborator

I actually wonder if it would be possible to remove _get_format_string() altogether... Make a new _get_data() or something like that, or inline a lot of the logic into the parse_value() function.

@greglucas greglucas mentioned this issue Sep 3, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants