drivers/lpsxxx: avoid float arithmetics #19609
Merged
Conversation
This file contains 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
maribu
added
Impact: minor
19 tasks
aabadie
requested changes
May 18, 2023
There was a problem hiding this comment.
Something is wrong with this PR. I just tested it on iotlab and the temperature returned by the test application is wrong:
$ make -C tests/drivers/lpsxxx/ flash term BOARD=iotlab-m3 IOTLAB_NODE=auto
make: Entering directory '/work/riot/RIOT/tests/drivers/lpsxxx'
Building application "tests_lpsxxx" for "iotlab-m3" with MCU "stm32".
"make" -C /work/riot/RIOT/pkg/cmsis/
"make" -C /work/riot/RIOT/boards/common/init
"make" -C /work/riot/RIOT/boards/iotlab-m3
"make" -C /work/riot/RIOT/boards/common/iotlab
"make" -C /work/riot/RIOT/core
"make" -C /work/riot/RIOT/core/lib
"make" -C /work/riot/RIOT/cpu/stm32
"make" -C /work/riot/RIOT/cpu/cortexm_common
"make" -C /work/riot/RIOT/cpu/cortexm_common/periph
"make" -C /work/riot/RIOT/cpu/stm32/periph
"make" -C /work/riot/RIOT/cpu/stm32/stmclk
"make" -C /work/riot/RIOT/cpu/stm32/vectors
"make" -C /work/riot/RIOT/drivers
"make" -C /work/riot/RIOT/drivers/lpsxxx
"make" -C /work/riot/RIOT/drivers/periph_common
"make" -C /work/riot/RIOT/sys
"make" -C /work/riot/RIOT/sys/auto_init
"make" -C /work/riot/RIOT/sys/div
"make" -C /work/riot/RIOT/sys/frac
"make" -C /work/riot/RIOT/sys/libc
"make" -C /work/riot/RIOT/sys/malloc_thread_safe
"make" -C /work/riot/RIOT/sys/newlib_syscalls_default
"make" -C /work/riot/RIOT/sys/pm_layered
"make" -C /work/riot/RIOT/sys/preprocessor
"make" -C /work/riot/RIOT/sys/stdio_uart
"make" -C /work/riot/RIOT/sys/ztimer
text data bss dec hex filename
12596 156 2324 15076 3ae4 /work/riot/RIOT/tests/drivers/lpsxxx/bin/iotlab-m3/tests_lpsxxx.elf
iotlab-node --jmespath='keys(@)[0]' --format='lambda ret: exit(int(ret))' --list saclay,m3,10 --flash /work/riot/RIOT/tests/drivers/lpsxxx/bin/iotlab-m3/tests_lpsxxx.bin
ssh -t abadie@saclay.iot-lab.info 'socat - tcp:m3-10.saclay.iot-lab.info:20000'
Pressure value: 1006hPa - Temperature: -263.-67°C
Pressure value: 1006hPa - Temperature: -263.-70°C
Pressure value: 1006hPa - Temperature: -263.-66°C
Pressure value: 1006hPa - Temperature: -263.-64°C
Pressure value: 1006hPa - Temperature: -263.-63°C
Pressure value: 1006hPa - Temperature: -263.-59°C
Pressure value: 1006hPa - Temperature: -263.-61°C
Pressure value: 1006hPa - Temperature: -263.-57°C
Pressure value: 1006hPa - Temperature: -263.-56°C
it works as expected on master:
$ make -C tests/drivers/lpsxxx/ flash term BOARD=iotlab-m3 IOTLAB_NODE=auto
make: Entering directory '/work/riot/RIOT/tests/drivers/lpsxxx'
Building application "tests_lpsxxx" for "iotlab-m3" with MCU "stm32".
"make" -C /work/riot/RIOT/pkg/cmsis/
"make" -C /work/riot/RIOT/boards/common/init
"make" -C /work/riot/RIOT/boards/iotlab-m3
"make" -C /work/riot/RIOT/boards/common/iotlab
"make" -C /work/riot/RIOT/core
"make" -C /work/riot/RIOT/core/lib
"make" -C /work/riot/RIOT/cpu/stm32
"make" -C /work/riot/RIOT/cpu/cortexm_common
"make" -C /work/riot/RIOT/cpu/cortexm_common/periph
"make" -C /work/riot/RIOT/cpu/stm32/periph
"make" -C /work/riot/RIOT/cpu/stm32/stmclk
"make" -C /work/riot/RIOT/cpu/stm32/vectors
"make" -C /work/riot/RIOT/drivers
"make" -C /work/riot/RIOT/drivers/lpsxxx
"make" -C /work/riot/RIOT/drivers/periph_common
"make" -C /work/riot/RIOT/sys
"make" -C /work/riot/RIOT/sys/auto_init
"make" -C /work/riot/RIOT/sys/div
"make" -C /work/riot/RIOT/sys/frac
"make" -C /work/riot/RIOT/sys/libc
"make" -C /work/riot/RIOT/sys/malloc_thread_safe
"make" -C /work/riot/RIOT/sys/newlib_syscalls_default
"make" -C /work/riot/RIOT/sys/pm_layered
"make" -C /work/riot/RIOT/sys/preprocessor
"make" -C /work/riot/RIOT/sys/stdio_uart
"make" -C /work/riot/RIOT/sys/ztimer
text data bss dec hex filename
13860 156 2324 16340 3fd4 /work/riot/RIOT/tests/drivers/lpsxxx/bin/iotlab-m3/tests_lpsxxx.elf
iotlab-node --jmespath='keys(@)[0]' --format='lambda ret: exit(int(ret))' --list saclay,m3,10 --flash /work/riot/RIOT/tests/drivers/lpsxxx/bin/iotlab-m3/tests_lpsxxx.bin
ssh -t abadie@saclay.iot-lab.info 'socat - tcp:m3-10.saclay.iot-lab.info:20000'
Pressure value: 1006hPa - Temperature: 44.46°C
Pressure value: 1006hPa - Temperature: 44.47°C
Pressure value: 1006hPa - Temperature: 44.45°C
Pressure value: 1006hPa - Temperature: 44.46°C
Pressure value: 1006hPa - Temperature: 44.46°C
Pressure value: 1006hPa - Temperature: 44.49°C
Still on iotlab, the nrf52dk boards deployed in Saclay have a ST shield with an lps22hb. The temperature value read is also wrong:
$ make -C tests/drivers/lpsxxx/ flash term BOARD=nrf52dk DRIVER=lps22hb IOTLAB_NODE=auto
make: Entering directory '/work/riot/RIOT/tests/drivers/lpsxxx'
Building application "tests_lpsxxx" for "nrf52dk" with MCU "nrf52".
"make" -C /work/riot/RIOT/pkg/cmsis/
"make" -C /work/riot/RIOT/boards/common/init
"make" -C /work/riot/RIOT/boards/nrf52dk
"make" -C /work/riot/RIOT/boards/common/nrf52xxxdk
"make" -C /work/riot/RIOT/core
"make" -C /work/riot/RIOT/core/lib
"make" -C /work/riot/RIOT/cpu/nrf52
"make" -C /work/riot/RIOT/cpu/cortexm_common
"make" -C /work/riot/RIOT/cpu/cortexm_common/periph
"make" -C /work/riot/RIOT/cpu/nrf52/periph
"make" -C /work/riot/RIOT/cpu/nrf52/vectors
"make" -C /work/riot/RIOT/cpu/nrf5x_common
"make" -C /work/riot/RIOT/cpu/nrf5x_common/periph
"make" -C /work/riot/RIOT/drivers
"make" -C /work/riot/RIOT/drivers/lpsxxx
"make" -C /work/riot/RIOT/drivers/periph_common
"make" -C /work/riot/RIOT/sys
"make" -C /work/riot/RIOT/sys/auto_init
"make" -C /work/riot/RIOT/sys/div
"make" -C /work/riot/RIOT/sys/frac
"make" -C /work/riot/RIOT/sys/libc
"make" -C /work/riot/RIOT/sys/malloc_thread_safe
"make" -C /work/riot/RIOT/sys/newlib_syscalls_default
"make" -C /work/riot/RIOT/sys/preprocessor
"make" -C /work/riot/RIOT/sys/stdio_uart
"make" -C /work/riot/RIOT/sys/ztimer
text data bss dec hex filename
12156 148 2636 14940 3a5c /work/riot/RIOT/tests/drivers/lpsxxx/bin/nrf52dk/tests_lpsxxx.elf
iotlab-node --jmespath='keys(@)[0]' --format='lambda ret: exit(int(ret))' --list saclay,nrf52dk,10 --flash /work/riot/RIOT/tests/drivers/lpsxxx/bin/nrf52dk/tests_lpsxxx.bin
ssh -t abadie@saclay.iot-lab.info 'socat - tcp:nrf52dk-10.saclay.iot-lab.info:20000'
Pressure value: 1005hPa - Temperature: 1.48°C
Pressure value: 1005hPa - Temperature: 1.50°C
Pressure value: 1005hPa - Temperature: 1.51°C
Pressure value: 1005hPa - Temperature: 1.51°C
Whereas it's correct on master:
$ make -C tests/drivers/lpsxxx/ flash term BOARD=nrf52dk DRIVER=lps22hb IOTLAB_NODE=auto
make: Entering directory '/work/riot/RIOT/tests/drivers/lpsxxx'
Building application "tests_lpsxxx" for "nrf52dk" with MCU "nrf52".
"make" -C /work/riot/RIOT/pkg/cmsis/
"make" -C /work/riot/RIOT/boards/common/init
"make" -C /work/riot/RIOT/boards/nrf52dk
"make" -C /work/riot/RIOT/boards/common/nrf52xxxdk
"make" -C /work/riot/RIOT/core
"make" -C /work/riot/RIOT/core/lib
"make" -C /work/riot/RIOT/cpu/nrf52
"make" -C /work/riot/RIOT/cpu/cortexm_common
"make" -C /work/riot/RIOT/cpu/cortexm_common/periph
"make" -C /work/riot/RIOT/cpu/nrf52/periph
"make" -C /work/riot/RIOT/cpu/nrf52/vectors
"make" -C /work/riot/RIOT/cpu/nrf5x_common
"make" -C /work/riot/RIOT/cpu/nrf5x_common/periph
"make" -C /work/riot/RIOT/drivers
"make" -C /work/riot/RIOT/drivers/lpsxxx
"make" -C /work/riot/RIOT/drivers/periph_common
"make" -C /work/riot/RIOT/sys
"make" -C /work/riot/RIOT/sys/auto_init
"make" -C /work/riot/RIOT/sys/div
"make" -C /work/riot/RIOT/sys/frac
"make" -C /work/riot/RIOT/sys/libc
"make" -C /work/riot/RIOT/sys/malloc_thread_safe
"make" -C /work/riot/RIOT/sys/newlib_syscalls_default
"make" -C /work/riot/RIOT/sys/preprocessor
"make" -C /work/riot/RIOT/sys/stdio_uart
"make" -C /work/riot/RIOT/sys/ztimer
text data bss dec hex filename
12152 148 2636 14936 3a58 /work/riot/RIOT/tests/drivers/lpsxxx/bin/nrf52dk/tests_lpsxxx.elf
iotlab-node --jmespath='keys(@)[0]' --format='lambda ret: exit(int(ret))' --list saclay,nrf52dk,10 --flash /work/riot/RIOT/tests/drivers/lpsxxx/bin/nrf52dk/tests_lpsxxx.bin
ssh -t abadie@saclay.iot-lab.info 'socat - tcp:nrf52dk-10.saclay.iot-lab.info:20000'
Pressure value: 1005hPa - Temperature: 27.62°C
Pressure value: 1005hPa - Temperature: 27.66°C
Pressure value: 1005hPa - Temperature: 27.67°C
Pressure value: 1005hPa - Temperature: 27.69°C
The 20° difference with the M3 comes from the fact that the M3s are in a closed box with an embedded Linux close/connected to it and it can be pretty hot inside. The nrf52dk are outside of the box :)
aabadie
reviewed
May 18, 2023
| /* compute actual temperature value in °C */ | ||
| res += ((float)val) / TEMP_DIVIDER; | ||
| /* convert val to c°C */ | ||
| val *= 100; |
There was a problem hiding this comment.
Could it be that since val is int16 there's an overflow when multiplying by 100 ?
There was a problem hiding this comment.
Could it be that since val is int16 there's an overflow when multiplying by 100 ?
Yes that's it. With the following diff, I get correct temperature values:
diff --git a/drivers/lpsxxx/lpsxxx.c b/drivers/lpsxxx/lpsxxx.c
index e0047c6cae..7019a94b42 100644
--- a/drivers/lpsxxx/lpsxxx.c
+++ b/drivers/lpsxxx/lpsxxx.c
@@ -131,7 +131,7 @@ int lpsxxx_init(lpsxxx_t *dev, const lpsxxx_params_t * params)
int lpsxxx_read_temp(const lpsxxx_t *dev, int16_t *temp)
{
uint8_t tmp;
- int16_t val = 0;
+ int32_t val = 0;
uint16_t res = TEMP_BASE; /* reference value -> see datasheet */
i2c_acquire(DEV_I2C);There was a problem hiding this comment.
Thanks for testing, I applied the bugfix :)
There was a problem hiding this comment.
not sure which target you are going for but assuming uint32_t val;
turning all of it into one multiplication might be faster.
(val *((100L<<16)/480))>>16)
There was a problem hiding this comment.
PRs are always welcome :)
aabadie
approved these changes
May 19, 2023
|
bors merge |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
thx :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Contribution description
Perform computation directly in centi-degree-celsius to avoid floating point arithmetics. In addition, use scientific rounding in the division.
Testing procedure
Beware: Code is untested. This should change results (much).
Issues/PRs references
Fixes #17486