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

Fix QMP6988 to work with AVR boards #4

Open
Erdien opened this issue Aug 4, 2022 · 2 comments
Open

Fix QMP6988 to work with AVR boards #4

Erdien opened this issue Aug 4, 2022 · 2 comments

Comments

@Erdien
Copy link

Erdien commented Aug 4, 2022

I'm working on ENV.III sensor with Arduino Uno and RaspberryPi Pico boards.
I found - the Pressure and Temperature from QMP6988 were incorrect because of 2 things:

1-st Issue:

M5Unit-ENV/src/QMP6988.h

Lines 12 to 13 in b1b5692

#define QMP6988_U32_t unsigned int
#define QMP6988_S32_t int

int on avr boards is int16_t.
long on both avr and 32-bit boards is int32_t

 #define QMP6988_U32_t unsigned long
 #define QMP6988_S32_t long

2-nd Issue:

qmp6988.qmp6988_cali.COE_a0 =
(QMP6988_S32_t)(((a_data_uint8_tr[18] << SHIFT_LEFT_12_POSITION) |
(a_data_uint8_tr[19] << SHIFT_LEFT_4_POSITION) |
(a_data_uint8_tr[24] & 0x0f))
<< 12);
qmp6988.qmp6988_cali.COE_a0 = qmp6988.qmp6988_cali.COE_a0 >> 12;
qmp6988.qmp6988_cali.COE_a1 =
(QMP6988_S16_t)(((a_data_uint8_tr[20]) << SHIFT_LEFT_8_POSITION) |
a_data_uint8_tr[21]);
qmp6988.qmp6988_cali.COE_a2 =
(QMP6988_S16_t)(((a_data_uint8_tr[22]) << SHIFT_LEFT_8_POSITION) |
a_data_uint8_tr[23]);
qmp6988.qmp6988_cali.COE_b00 =
(QMP6988_S32_t)(((a_data_uint8_tr[0] << SHIFT_LEFT_12_POSITION) |
(a_data_uint8_tr[1] << SHIFT_LEFT_4_POSITION) |
((a_data_uint8_tr[24] & 0xf0) >>
SHIFT_RIGHT_4_POSITION))
<< 12);

a_data_uint8_tr[18] and a_data_uint8_tr[0] after leftshifting these values by 12 - they'll overflow on avr boards.
changing their type to QMP6988_S32_t will solve this issue

 qmp6988.qmp6988_cali.COE_a0 = 
     (QMP6988_S32_t)((((QMP6988_S32_t)a_data_uint8_tr[18] << SHIFT_LEFT_12_POSITION) | 
                      (a_data_uint8_tr[19] << SHIFT_LEFT_4_POSITION) | 
                      (a_data_uint8_tr[24] & 0x0f)) 
                     << 12); 
 qmp6988.qmp6988_cali.COE_a0 = qmp6988.qmp6988_cali.COE_a0 >> 12; 
  
 qmp6988.qmp6988_cali.COE_a1 = 
     (QMP6988_S16_t)(((a_data_uint8_tr[20]) << SHIFT_LEFT_8_POSITION) | 
                     a_data_uint8_tr[21]); 
 qmp6988.qmp6988_cali.COE_a2 = 
     (QMP6988_S16_t)(((a_data_uint8_tr[22]) << SHIFT_LEFT_8_POSITION) | 
                     a_data_uint8_tr[23]); 
  
 qmp6988.qmp6988_cali.COE_b00 = 
     (QMP6988_S32_t)((((QMP6988_S32_t)a_data_uint8_tr[0] << SHIFT_LEFT_12_POSITION) | 
                      (a_data_uint8_tr[1] << SHIFT_LEFT_4_POSITION) | 
                      ((a_data_uint8_tr[24] & 0xf0) >> 
                       SHIFT_RIGHT_4_POSITION)) 
                     << 12); 
@Erdien Erdien changed the title Fix QMP6988 to work with 8-bit AVR boards Fix QMP6988 to work with AVR boards Aug 4, 2022
@tvillingett
Copy link

How is this for the RISC-V (ESP32-C3) processors, could this be a problem here also?

@Erdien
Copy link
Author

Erdien commented Jan 20, 2023

I haven't tested the code on any version of ESP32, so there is probably no problem with RISC-V or Xtensa LXx processors.
If this code were to be validated, it would have to be tested on the appropriate boards first.
I reported the bug with the idea that the type of the variables should be strict, to reduce the possibility of misinterpretation by compilers for other (non-ESP32) platforms.

To be clear: The main reason of the suggestion is, that the size of an int varies depending on the bitness of the processor (an int weighs 2 bytes on an AVR (8-bit) processor and 4 bytes on an ARM/RISC-V (32-bit) processor).
In the first part, I wrote about changing data types to ones that are more likely to be the same size whether you used an ESP32 board or an Arduino board.
I think, this suggestion doesn't changes much for the ESP32 board, but it might make it easier for Arduino users to use ENV modules in their projects when they don't have to pay attention to changing a few lines of code.

I haven't found any other implementation of this driver, except for this implementation, but it doesn't work with the Arduino Uno without modifications too.

It is also possible to use fixed-width integers for QMP6988_Xxx_t elements to ensure that they will be the same size regardless of architecture.

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

No branches or pull requests

2 participants