STMicroelectronics / lis2dw12-pid

lis2dw12 platform independent driver based on Standard C language and compliant with MISRA standard
BSD 3-Clause "New" or "Revised" License
11 stars 4 forks source link

Bug in lis2dw12_acceleration_raw_get #1

Closed prashant091088 closed 3 years ago

prashant091088 commented 3 years ago

The function "lis2dw12_acceleration_raw_get" is giving incorrect sensor results along X-, Y- and Z-axis since it doesn't ignore 2 Least significant bits on OUT_X_L (28h), OUT_Y_L (2Ah) and OUT_Z_L (2Ch).

** Current Implementation **: val[0] = (int16_t)buff[1]; val[0] = (val[0] 256) + (int16_t)buff[0]; val[1] = (int16_t)buff[3]; val[1] = (val[1] 256) + (int16_t)buff[2]; val[2] = (int16_t)buff[5]; val[2] = (val[2] * 256) + (int16_t)buff[4];

** Actual implementation must be ** val[0] = (int16_t)buff[1]; val[0] = (int16_t)((val[0] << 8) + buff[0]); val[0] = (int16_t)(val[0] >> 2);

val[1] = (int16_t)buff[3]; val[1] = (int16_t)((val[1] << 8) + buff[2]); val[1] = (int16_t)(val[1] >> 2);

val[2] = (int16_t)buff[5]; val[2] = (int16_t)((val[2] << 8) + buff[4]); val[2] = (int16_t)(val[2] >> 2);

avisconti commented 3 years ago

The function "lis2dw12_acceleration_raw_get" is giving incorrect sensor results along X-, Y- and Z-axis since it doesn't ignore 2 Least significant bits on OUT_X_L (28h), OUT_Y_L (2Ah) and OUT_Z_L (2Ch).

** Current Implementation **: val[0] = (int16_t)buff[1]; val[0] = (val[0] 256) + (int16_t)buff[0]; val[1] = (int16_t)buff[3]; val[1] = (val[1] 256) + (int16_t)buff[2]; val[2] = (int16_t)buff[5]; val[2] = (val[2] * 256) + (int16_t)buff[4];

** Actual implementation must be ** val[0] = (int16_t)buff[1]; val[0] = (int16_t)((val[0] << 8) + buff[0]); val[0] = (int16_t)(val[0] >> 2);

val[1] = (int16_t)buff[3]; val[1] = (int16_t)((val[1] << 8) + buff[2]); val[1] = (int16_t)(val[1] >> 2);

val[2] = (int16_t)buff[5]; val[2] = (int16_t)((val[2] << 8) + buff[4]); val[2] = (int16_t)(val[2] >> 2);

@prashant091088

The lis2dw12_acceleration_raw_get() returns the raw values read out from the sensors. The value must be shifted after the raw_get() call. If you see the lis2dw12_STdC/examples/lis2dw12_read_data_polling.c file there is the following code:

      lis2dw12_acceleration_raw_get(&dev_ctx, data_raw_acceleration);
      acceleration_mg[0] = lis2dw12_from_fs2_to_mg(
                             data_raw_acceleration[0]);
      acceleration_mg[1] = lis2dw12_from_fs2_to_mg(
                             data_raw_acceleration[1]);
      acceleration_mg[2] = lis2dw12_from_fs2_to_mg(
                             data_raw_acceleration[2]);

The implementaion of lis2dw12_from_fs2_to_mg() is like this:

float_t lis2dw12_from_fs2_to_mg(int16_t lsb)
{
  return ((float_t)lsb) * 0.061f;
}

0.061f is used instead of 0.244f, because the ">> 2" shift that you are talking about.

Does it make sense to you?

prashant091088 commented 3 years ago

@avisconti

Thanks for your clarification.

and just for suggestion, we can save some CPU cycles by using left shifting instead of multiplying by 256.

-- val[0] = (val[0] * 256) + (int16_t)buff[0];

++ val[0] = (val[0] << 8) + (int16_t)buff[0];

avisconti commented 3 years ago

@avisconti

Thanks for your clarification.

and just for suggestion, we can save some CPU cycles by using left shifting instead of multiplying by 256.

-- val[0] = (val[0] * 256) + (int16_t)buff[0];

++ val[0] = (val[0] << 8) + (int16_t)buff[0];

@prashant091088 Yes, I was about to replying also on this yesterday, but after few checks I had some doubts. Let me explain. I have been told that the reason for using *256 insteade of bitwise operation is because the former method guarantees that it works for both endiannes. But after looking on Google I was not so sure. I will keep investigating on this anyway. Thanks for suggestion.