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

Swap the endianness of accelerometer values read from device #33

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

hnez
Copy link

@hnez hnez commented Jun 12, 2020

Hi,

when testing on an ADXL345 accelerometer, that shares a device-id with the ADXL343 and looks to be mostly register-compatible, the measurements returned by accel_norm() and accel_raw() do not look quite right.

The datasheet for both ADXL345 and ADXL343 states, that:

The output data is twos complement, with DATAx0 as the least significant byte and DATAx1 as the most significant byte

When treated as little endian i16 instead of as a big endian i16 the resulting accelerometer readings look more sensible.

Please note that I do not own an actual ADXL343 to test this patch on and that my assumtion that this patch should also work for that accelerometer is only based on the very similar datasheets.

When testing on an ADXL345 accelerometer, that shares a device-id with
the ADXL343 and looks mostly register-compatible, the measurements
returned by accel_norm() and accel_raw() do not look right.

The datasheet for both ADXL345 and ADXL343 states, that

> The outputdata is twos complement, with DATA x 0 as the
> least significant byte and DATA x 1 as the most significant byte

e.g. little endian. Change lib.rs to reflect this endianness.
@tarcieri
Copy link
Contributor

Interesting! Let me test it out on my side but it looks like you are correct

@tarcieri
Copy link
Contributor

tarcieri commented Jun 13, 2020

I tried this out. Unfortunately I don't have SWD set up right now (I can look into reporting the data via USB instead), however outputting it directly to some NeoPixels it looks like noise, as opposed to gradiations (well, flickering gradiations) as I rotate the device with the existing implementation, so at the very least something else is wrong.

I went through the code to give myself a refresher (as it were, the comment for write_read_i16 quotes the same section of the manual you did, although the existing implementation does not appear to match the comment), and I think there's a little bit more going on here, namely the "Justify Bit":

Justify Bit
A setting of 1 in the justify bit selects left-justified (MSB) mode,
and a setting of 0 selects right-justified mode with sign extension.

The current implementation maps these onto different endianness interpretations depending on whether or not the justify bit is set. They take the form of I16x3 vs U16x3-based impls of RawAccelerometer, where the I16x3 version presently uses the big endian interpretation of the output, and U16x3 uses the little endian interpretation.

tl;dr: while I think you're right and this PR should be merged, there's more going on here, and this change alone breaks the driver.

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 this pull request may close these issues.

2 participants