Dhruvu/hcsro4 driver #155
Dhruvu/hcsro4 driver #155
Conversation
sahil-kale
requested review from
Pop0097,
SnackkOverflowError and
sahil-kale
sahil-kale
left a comment
sahil-kale
left a comment
There was a problem hiding this comment.
I feel as though the singleton design pattern is not correctly implemented here, as the function names seem misleading and "data" is being used to store initial configurations.
|
|
||
| /** | ||
| * Callback for exclusive use by the SPI ISR. | ||
| typedef struct ultrasonicData_t { | ||
| float distance; | ||
| bool isDataNew; // Records whether the data has been refreshed since the last time getResult was called | ||
| GPIO_TypeDef * triggerPin; | ||
| GPIO_TypeDef * echoPin; | ||
| TIM_HandleTypeDef htim; | ||
| } ultrasonicData_t; |
There was a problem hiding this comment.
Myabe add the timestamp as well into this
| #include <cstdint> | ||
|
|
||
| #define SPEED_OF_SOUND 0.034f | ||
| #define TRIG_PIN 5 // temporary value, to be changed when pin configs completed |
There was a problem hiding this comment.
where would trig pin be dfined then if it's in the ultrasonic struct?
| uint32_t IC_Val1 = 0; | ||
| uint32_t IC_Val2 = 0; | ||
| uint32_t difference = 0; | ||
| uint8_t isFirstCaptured = 0; // Is the first value captured | ||
| uint8_t distance = 0; // computed distance based on the length of the ECHO |
There was a problem hiding this comment.
Actually, these look like variables that should be part of the class
| HCSR04(const HCSR04*) = delete; | ||
| void getDistance(ultrasonicData_t * data); | ||
|
|
There was a problem hiding this comment.
I think you're missing a getInstance function here...
There was a problem hiding this comment.
yeah, we want to use the singleton pattern here. Take a look at the other sensor drivers for reference
| } | ||
|
|
||
| void HCSR04::delayMicroseconds(uint32_t us) { | ||
| int16_t currentTim = htim11.Instance -> CNT; |
| void HCSR04 :: getDistance(ultrasonicData_t * data) { | ||
| trigger(data -> triggerPin, TRIG_PIN); // Sends trigger pulse | ||
| __HAL_TIM_ENABLE_IT(data -> htim, TIM_IT_CC1); | ||
| } |
There was a problem hiding this comment.
This function feels misleading, it doesn't actually update the data, just triggers the measurement. I'd expect getDistance to update the data, please clarify in comment or rename function
| difference = IC_Val2 - IC_Val1; | ||
| } | ||
| else if (IC_Val1 > IC_Val2) { | ||
| difference = (0xffff - IC_Val1) + IC_Val2; // find difference between val1 and max, and add overflow val2 |
There was a problem hiding this comment.
Please make the 0xffff a constant of sorts in a define to something.
Also, what are the timing constraints on the timer? Max pulse width is 38ms, so there sohuldn't be an overflow if we're smart about how we start the timer
| #include "stm32f765xx.h" | ||
| #include "stm32f7xx_hal_tim.h" |
There was a problem hiding this comment.
Let's include this in the .cpp file so any file that includes ultrasonic.hpp is still unit testable
| HCSR04(const HCSR04*) = delete; | ||
| void getDistance(ultrasonicData_t * data); | ||
|
|
There was a problem hiding this comment.
yeah, we want to use the singleton pattern here. Take a look at the other sensor drivers for reference
Autopilot/Inc/ultrasonic.hpp
Outdated
| void trigger(GPIO_TypeDef * triggerPin, uint32_t pinName); | ||
| void delayMicroseconds(uint32_t us); | ||
|
|
||
| }; No newline at end of file |
There was a problem hiding this comment.
@Pop0097 Can we get clang-format working for our repo?
3 participants
Hey guys, this is the driver rewrite for the ultrasonic sensor we plan to use in our drone, the HC-SR04, which is the one found in most generic Arduino kits.
I've written the header file for it and would love some feedback before I implement the source file!
Here's the datasheet for it if you would like to review: https://cdn.sparkfun.com/datasheets/Sensors/Proximity/HCSR04.pdf