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

ST LIS2DUX12 driver: will not correctly read values on big-endian HW #75758

Closed
ragrus-nbl opened this issue Jul 11, 2024 · 2 comments · Fixed by #76557
Closed

ST LIS2DUX12 driver: will not correctly read values on big-endian HW #75758

ragrus-nbl opened this issue Jul 11, 2024 · 2 comments · Fixed by #76557
Assignees
Labels
area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@ragrus-nbl
Copy link

Describe the bug
ST LIS2DUX12 accelerometer driver makes unnecessary "little-endian to CPU endian" conversion. This will lead to incorrect readings on big-endian HW.

I believe these sys_le16_to_cpu() conversions are not needed: three here and one there.

That's because values are converted to CPU endianness in the hal_st code: here and there correspondingly.

I can't reproduce/check this in practice as I don't have a big-endian HW with this sensor.

To Reproduce
Read any sensor channel (e.g., SENSOR_CHAN_DIE_TEMP) on big-endian HW.

Environment

  • Zephyr 3.7.0-rc2
@ragrus-nbl ragrus-nbl added the bug The issue is a bug, or the PR is fixing a bug label Jul 11, 2024
@nashif nashif added the priority: low Low impact/importance bug label Jul 11, 2024
@teburd teburd assigned avisconti and unassigned teburd Jul 16, 2024
@teburd teburd added the platform: STM32 ST Micro STM32 label Jul 16, 2024
@avisconti
Copy link
Collaborator

avisconti commented Jul 25, 2024

Well, the BE/LE stuff is always confusing me.

The STDC drivers (included in zephyr as stmemsc in hal_st module) have indeed the purpose to abstract from the platform details. So, they should in fact already solve the BE/LE related issue, and I would like to check it on some BE platform (I do not have any) to see whether this is really the case. But I agree that from zephyr driver standpoint that call (i.e sys_le16_to_cpu()) should not be there as the stmemsc HAL should already take care of the byte swapping as I mentioned earlier...

Let me think more about it. Thanks for pointing this out.

@erwango erwango removed the platform: STM32 ST Micro STM32 label Jul 31, 2024
@erwango
Copy link
Member

erwango commented Jul 31, 2024

Removing STM32 label as this is not related to STM32 SoC.

avisconti added a commit to avisconti/zephyr that referenced this issue Aug 27, 2024
A 16-bit value built using byte shifts and ORs from a given
couple of lsb and msb bytes will result to be the same on both
little-endian and big-endian architectures, e.g.

    uint8_t lsb, msb;
    int16_t val;

    /* val is the same number on both le and be archs, but has
       different layout in memory */
    val = (msb << 8) | lsb;

All the xyz_raw_get() APIs of stmemsc sensor module build the sensor
data using the above method and DO NOT hence require (it actually leads
to wrong values on big-endian machines) to use any le/be swap routines,
such as sys_le16_to_cpu().

Fix zephyrproject-rtos#75758

Signed-off-by: Armando Visconti <[email protected]>
@nashif nashif closed this as completed in 9ea4cb9 Aug 27, 2024
zephyrbot pushed a commit that referenced this issue Aug 27, 2024
A 16-bit value built using byte shifts and ORs from a given
couple of lsb and msb bytes will result to be the same on both
little-endian and big-endian architectures, e.g.

    uint8_t lsb, msb;
    int16_t val;

    /* val is the same number on both le and be archs, but has
       different layout in memory */
    val = (msb << 8) | lsb;

All the xyz_raw_get() APIs of stmemsc sensor module build the sensor
data using the above method and DO NOT hence require (it actually leads
to wrong values on big-endian machines) to use any le/be swap routines,
such as sys_le16_to_cpu().

Fix #75758

Signed-off-by: Armando Visconti <[email protected]>
(cherry picked from commit 9ea4cb9)
nashif pushed a commit that referenced this issue Aug 31, 2024
A 16-bit value built using byte shifts and ORs from a given
couple of lsb and msb bytes will result to be the same on both
little-endian and big-endian architectures, e.g.

    uint8_t lsb, msb;
    int16_t val;

    /* val is the same number on both le and be archs, but has
       different layout in memory */
    val = (msb << 8) | lsb;

All the xyz_raw_get() APIs of stmemsc sensor module build the sensor
data using the above method and DO NOT hence require (it actually leads
to wrong values on big-endian machines) to use any le/be swap routines,
such as sys_le16_to_cpu().

Fix #75758

Signed-off-by: Armando Visconti <[email protected]>
(cherry picked from commit 9ea4cb9)
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this issue Sep 6, 2024
A 16-bit value built using byte shifts and ORs from a given
couple of lsb and msb bytes will result to be the same on both
little-endian and big-endian architectures, e.g.

    uint8_t lsb, msb;
    int16_t val;

    /* val is the same number on both le and be archs, but has
       different layout in memory */
    val = (msb << 8) | lsb;

All the xyz_raw_get() APIs of stmemsc sensor module build the sensor
data using the above method and DO NOT hence require (it actually leads
to wrong values on big-endian machines) to use any le/be swap routines,
such as sys_le16_to_cpu().

Fix zephyrproject-rtos#75758

(cherry picked from commit 9ea4cb9)

Original-Signed-off-by: Armando Visconti <[email protected]>
GitOrigin-RevId: 9ea4cb9
Cr-Build-Id: 8738366835818535617
Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8738366835818535617
Copybot-Job-Name: zephyr-main-copybot-downstream
Change-Id: If4b94e59d1e13fea9433ad90e6a110484a175e5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5822371
Reviewed-by: Ting Shen <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Tested-by: Ting Shen <[email protected]>
Commit-Queue: Ting Shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants