libhackrf: fix endianness on max2837/max2831/rffc5071 register reads#1794
Closed
munzzyy wants to merge 1 commit into
Closed
libhackrf: fix endianness on max2837/max2831/rffc5071 register reads#1794munzzyy wants to merge 1 commit into
munzzyy wants to merge 1 commit into
Conversation
hackrf_max2837_read, hackrf_max2831_read, and hackrf_rffc5071_read cast the raw 2-byte USB control-transfer buffer straight to uint16_t* with no byte-order conversion. The firmware always puts the register value on the wire little-endian, so this only works out on little-endian hosts. hackrf_read_adc a bit further down does this correctly with FROM_LE16(), and so does hackrf_board_partid_serialno_read - these three reads are just missing it. Add the same *value = FROM_LE16(*value) call to all three. On little-endian hosts FROM_LE16 is a no-op so this changes nothing there. On big-endian hosts (s390x, some ARM/MIPS/PPC builds) it fixes register reads that are currently silently byte-swapped. Closes greatscottgadgets#778
Member
|
We do not accept LLM-generated contributions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #778.
hackrf_max2837_read,hackrf_max2831_read, andhackrf_rffc5071_readall cast the raw 2-byte USB control-transfer buffer directly touint16_t*and return it as-is, with no byte-order conversion. That only gives the right answer on a little-endian host, because the firmware always puts the register value on the wire little-endian.A few lines down,
hackrf_read_adcdoes the same kind of read but correctly converts with*value = FROM_LE16(*value);, andhackrf_board_partid_serialno_readdoes the same for its fields. TheFROM_LE16macro is already defined at the top ofhackrf.cand compiles to a no-op on little-endian,__builtin_bswap16on big-endian. These three reads just never got it applied.This came up in #778 back in 2020. The maintainer's reply at the time ("I think it's correct... it gets loaded into the buffer as little-endian on the firmware side") is true but answers a different question - the firmware side being little-endian is exactly why the host needs to swap on a big-endian host. The original reporter clarified this in a follow-up comment and said they'd send a PR, but it looks like that never happened, so here it is.
Change: add
*value = FROM_LE16(*value);to the success path of all three functions, matching the existing pattern inhackrf_read_adc.Testing: this is a no-op on little-endian hosts (x86/x86_64/aarch64/arm - the overwhelming majority of libhackrf builds), since
FROM_LE16expands toxthere, so there's no behavior change or regression risk for the common case. I don't have a big-endian machine or a HackRF on hand to test against right now, so I haven't been able to verify the swapped path against real hardware. The macro itself is already proven correct elsewhere in the same file (hackrf_read_adc,hackrf_board_partid_serialno_read), so this is applying an existing, working pattern rather than introducing new logic. Flagging that a spot-check on an actual big-endian build (or at least a unit test feeding a non-symmetric byte pair through the macro) would be good before merge if anyone has that hardware handy.