Refactored commit - VL53L4CX default settings #564
Conversation
|
@brentru ready for review, release blocked until partition size resolved on some build targets as discussed yesterday |
tyeth
force-pushed
the
add-VL53L4CX
branch
2 times, most recently
from
e0abaf8
to
213fcef
Compare
|
@brentru ready for review, tested locally, will retest on staging before merge |
|
Re-ready for review @brentru |
| */ | ||
| /*******************************************************************************/ | ||
| void awaitDataReady(int &status, uint8_t &NewDataReady) { | ||
| for (uint8_t retries = 0; |
There was a problem hiding this comment.
Could we return NAN, and return immediately vs retrying three times? These drivers are supposed to be very small and fast, like an interrupt routine.
| otherwise. | ||
| */ | ||
| /*******************************************************************************/ | ||
| bool getProximity(sensors_event_t *proximityEvent, int index = 0) { |
There was a problem hiding this comment.
I feel this function is doing many things and it's resulting in a conditional statement that's difficult for me to parse. Could we refactor this a bit?
I'll suggest some ideas below..
There was a problem hiding this comment.
Yeah it's not exactly friendly and easily readable, will have a rethink
| if ((status == VL53L4CX_ERROR_NONE) && (NewDataReady != 0)) { | ||
| status = _VL53L4CX->VL53L4CX_GetMultiRangingData(pMultiRangingData); | ||
| int no_of_object_found = pMultiRangingData->NumberOfObjectsFound; | ||
| if (no_of_object_found - 1 < index) { |
There was a problem hiding this comment.
This index boundary is confusing. Could you either reformat it, or refactor it out into a function that is then checked?
| proximityEvent->data[0] = NAN; | ||
| return true; | ||
| } | ||
| bool retVal = updateDataPointIfValid(pMultiRangingData->RangeData[index], |
There was a problem hiding this comment.
I'm unsure how you'd want to restructure the conditional, but this (L150-L152) is hidden within the nested if statement. It's the code's desired outcome, so, could we make it more visible?
No description provided.