stm32duino / VL53L4CD

Arduino library to support the VL53L4CD Time-of-Flight high accuracy proximity sensor
BSD 3-Clause "New" or "Revised" License
5 stars 7 forks source link

Fix broken check for sensor ID #11

Closed sebromero closed 1 month ago

sebromero commented 1 month ago

Summary

This PR fixes a bug that would make an incorrect sensor ID go unnoticed. This is because when the sensor ID is fetched successfully using VL53L4CD_GetSensorId() the first part of the following if clause evaluates to false because the return value will be VL53L4CD_ERROR_NONE which means the sensor ID is never checked. The sensor ID needs to be checked separately and return a dedicated error so it can be distinguished from a timeout error.

cparata commented 1 month ago

Hi @sebromero , I checked in the ULD software released by ST for this sensor and I cannot find any reference to "VL53L4CD_ERROR_INVALID_SENSOR_ID" error. I prefer to keep the library code aligned to ULD software provided by ST to make easier in future the update when it will be available. Regarding the check in the code, if the "VL53L4CD_GetSensorId()" returns a value different from "VL53L4CD_ERROR_NONE", it does not make sense to check the id value because it means that you had an issue on the I2C bus and so the read id value is not valid. In my opinion, the current check is robust enough even if it does not return a specific error if the ID is not the one expected. Best Regards, Carlo

sebromero commented 1 month ago

Hi Carlo! Fair point about the error code. However, the problem is not so much with the error code but the if clause is not correct. Let's break it down:

status = VL53L4CD_GetSensorId(&sensor_id);
// In case this was successful, status will be VL53L4CD_ERROR_NONE
// Let's imagine that the sensor_id is 0xeacc which is a different sensor than VL53L4CD
// So it should return an error

// Let's replace "status" with that "error" code VL53L4CD_ERROR_NONE
if (VL53L4CD_ERROR_NONE != VL53L4CD_ERROR_NONE && (sensor_id != 0xebaa)) {
  return VL53L4CD_ERROR_TIMEOUT;
}

// Let's replace the comparison with the boolean result of the expression
if (false && (sensor_id != 0xebaa)) {
  return VL53L4CD_ERROR_TIMEOUT;
}

// Since if(false && ...) is always false, it won't evaluate the second part of the expression
// and hence it won't step into the if block

// Code shouldn't reach here since the sensor_id is not 0xebaa
status = VL53L4CD_SensorInit();

You see, it doesn't catch the incorrect sensor ID. If we want to also use VL53L4CD_ERROR_TIMEOUT for this case we can just use a boolean OR. I modified the PR accordingly.

cparata commented 1 month ago

Hi @sebromero , thanks a lot for your fix! You are right. I integrated it in the repo. Best Regards, Carlo