VL53L4CD ULD : divide by 0 bug in VL53L4CD_GetResult

[julesr]

I am using VL53L4CX with VL53L4CD ULD driver v2.2.2, due to an I2C issue on my side the VL53L4CD_RdWord fails and the number_of_spad variable is set to 0. The divide by zero happen at the very end of VL53L4CD_GetResult.

I checked the function in ULD v2.2.3 and the number_of_spad value is still used for division without checks :

VL53L4CD_Error VL53L4CD_GetResult(
		Dev_t dev,
		VL53L4CD_ResultsData_t *p_result)
{
	VL53L4CD_Error status = VL53L4CD_ERROR_NONE;
	uint16_t temp_16;
	uint8_t temp_8;
	uint16_t raw_spads;
	
	uint8_t status_rtn[24] = { 255, 255, 255, 5, 2, 4, 1, 7, 3,
			0, 255, 255, 9, 13, 255, 255, 255, 255, 10, 6,
			255, 255, 11, 12 };

	status |= VL53L4CD_RdByte(dev, VL53L4CD_RESULT__RANGE_STATUS,
		&temp_8);
	temp_8 = temp_8 & (uint8_t)0x1F;
	if (temp_8 < (uint8_t)24)
	{
		temp_8 = status_rtn[temp_8];
	}
	p_result->range_status = temp_8;

	status |= VL53L4CD_RdWord(dev, VL53L4CD_RESULT__SPAD_NB,
		&temp_16);
	raw_spads=temp_16;
	p_result->number_of_spad = temp_16 / (uint16_t) 256;

	status |= VL53L4CD_RdWord(dev, VL53L4CD_RESULT__SIGNAL_RATE,
		&temp_16);
	p_result->signal_rate_kcps = (uint32_t)temp_16 * 8;

	status |= VL53L4CD_RdWord(dev, VL53L4CD_RESULT__AMBIENT_RATE,
		&temp_16);
	p_result->ambient_rate_kcps = (uint32_t)temp_16 * 8;

	status |= VL53L4CD_RdWord(dev, VL53L4CD_RESULT__SIGMA,
		&temp_16);
	p_result->sigma_mm = temp_16 / (uint16_t) 4;

	status |= VL53L4CD_RdWord(dev, VL53L4CD_RESULT__DISTANCE,
		&temp_16);
	p_result->distance_mm = temp_16;

	p_result->signal_per_spad_kcps = p_result->signal_rate_kcps *256
			/(uint32_t) raw_spads;
	p_result->ambient_per_spad_kcps = p_result->ambient_rate_kcps *256
			/(uint32_t) raw_spads;																							 

	return status;
}

 
According to me, this lacks an if statement. Easy bug to spot and avoid anyway, but at least it is reported !

[John_Kvam]

Did you get this error? 

The reason I ask is that there should be a minimum number of SPADs. And it's about 4. Might be a bit less as some SPADs are deliberately occluded to deal with situations with massive amounts of light. 

With no SPADs one can't get any photon detects and if this happens the sensor just cannot work. 

I suppose if something went wrong and the sensor quit prematurely, some of the data registers might not have been filled in. In this case maybe the number of SPADs could be zero I suppose. 

You ARE right in that it should be accounted for, but I'd hope the status would have indicated that the sensor got a bad result. 

- john

 

[Bin]

Hi:

About this, when the sensor starts up normally and loads the default register configuration, I did some test and found that even under high reflectivity mirrors, the minimum SPAD_Number of the sensor is 1, and the raw Spads num will reach 448.

But in your case, there is something wrong with I2C. As john side, the data registers might not have been filled in. In this case maybe the number of SPADs could be 0. At this point, the return status is definitely not 0 for normal distance measurement. so, we can analyze it specifically through the return status.

Of course, your suggestion is very useful, but this driver is designed for Ultra lite driver to simplify as much as possible. 

Thank you for your contribution to the ST community.

 

[Zhiyuan.Han]

Hi

 As Bin shared, with normal operation, you should never get 0 SPAD number, if you got 0 SPAD, this is mean something went wrong. as John shared, you can check the range status to confirm if the sensor is ranging correctly. 

We designed the driver as ultra-light version to help customer save memory space, so we didn't apply the divide 0 check as it should be never happened in normal operation.  

 

Br

Zhiyuan.Han

[julesr]

I am answering to all previous answers : from software perspective I very strongly advise to add an if statement. Otherwise you let to the user the possibility of a crash, this won't be any issue from memory perspective.

And on my side the crashed was caused by I2C issue + unfortunate zeroing the I2C response buffer after I2C failure (instead of just letting the random value that was in there). I did no experiment on more "advanced" failure hypothesis close to the sensor's HW, and nothing seems to cause any issue on that side.

Thank you very much for your answers,

Best

Jules