search

francisrstokes / construct-js

🛠️A library for creating byte level data structures.
MIT License
1.37k stars 29 forks source link

Ensure BitStructs are in expected (MSb to LSb) order #8

Closed revbingo closed 4 years ago

revbingo commented 4 years ago

Creating a buffer from a BitStruct results in the fields being rendered in reverse from the order they were specified. getBits() on a MultiBit field also renders the bits in reverse order, but to add to the confusion when rendered as part of the BitStruct, the reversal is reversed, so it's the right way round :).

Example:

const mb = dz.BitStruct('bits')
      .flag('a', false)
      .flag('b', true)
      .multiBit('c', 4, 10)

I would expect to render "left to right" a/b/c/[pad], so 0/1/1010/00, or 68 hex. The code as it stands generates 00/1010/1/0 (2a hex). Perhaps this is actually intentional, but seems a bit counterintuitive?

This pull request fixes both those issues to produce fields in the order in which they are specified, and also refactors to fix a bug in the toBytes() method that had a undefined field bits

Thanks for the library though, just what I'm needing :)

francisrstokes commented 4 years ago

Hey @revbingo - thanks for the PR, and glad to hear that the library is useful for you! I think (and it's been a while so I don't remember completely 😄 ) that my original intention is that a would be the 0th thing, and thus would appear in the 0th bit of the struct.

Clearly there's call for both! So maybe what would be the most useful approach would be to allow defining the bit ordering to be defined as the second argument to BitStruct - just like endianness can be defined for regular Struct?

What do you think?

revbingo commented 4 years ago

I thought that might be the case, I see the thinking, it just felt odd when I was trying to layout a payload e.g. field1, field2, field3, then suddenly having to specify bit fields in reverse. So yeah, I think a flag would be good, although I'm struggling a bit with naming - perhaps lsbFirst? If true, the first field specified is the least significant bit (i.e. your current order). If false, the first field is treated as MSB i.e. my preferred ordering.

francisrstokes commented 4 years ago

Sounds great to me - least significant bit as the default matches nicely with the little endian default too.

francisrstokes commented 4 years ago

Thanks @revbingo 🎉 great little feature to have! It's published in v0.3.0