Add VREF wrapper to BSP#266
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the VS Code configuration for ARM-based Linux development, adds a function to retrieve the ADC reference voltage, and introduces a comprehensive CAN message header. Feedback identifies a bug in the adc_get_vref implementation where a channel macro is returned instead of a voltage value, unprofessional naming in the CAN header, incomplete test logic for ADC readings, and portability issues stemming from absolute paths in the workspace settings.
| uint32_t adc_get_vref(void) { | ||
| // this wasn't my doing | ||
| return ADC_CHANNEL_VREFINT; | ||
| } |
There was a problem hiding this comment.
The function adc_get_vref is documented to return the reference voltage for scaling calculations, but it currently returns ADC_CHANNEL_VREFINT. In the STM32 HAL, ADC_CHANNEL_VREFINT is a macro used to select the internal reference channel (typically a bitmask or index), not a voltage value. This will cause incorrect results in any scaling logic. It should return the actual reference voltage (e.g., 3300 for 3.3V).
uint32_t adc_get_vref(void) {
return 3300; // Return nominal VREF in mV
}| } controls_status_controls_leader_fault_e; | ||
|
|
||
| typedef enum { | ||
| BBPDU_SET_SWITCHES_HSS_CONTROL_13_DO_NOTHING_YOU_RE_STUPID = 3, |
There was a problem hiding this comment.
The enum member name BBPDU_SET_SWITCHES_HSS_CONTROL_13_DO_NOTHING_YOU_RE_STUPID is unprofessional. Please rename it to something more descriptive and neutral, such as BBPDU_SET_SWITCHES_HSS_CONTROL_13_NO_CHANGE. This naming pattern appears multiple times throughout the file.
| BBPDU_SET_SWITCHES_HSS_CONTROL_13_DO_NOTHING_YOU_RE_STUPID = 3, | |
| BBPDU_SET_SWITCHES_HSS_CONTROL_13_NO_CHANGE = 3, |
| } | ||
| } | ||
|
|
||
| volatile uint32_t scaled_reading; |
There was a problem hiding this comment.
The variable scaled_reading is declared but never assigned a value or used in any logic. This suggests that the test for the new scaling functionality is incomplete.
| "${workspaceFolder}/**", // Your project headers | ||
| "/usr/include", // System headers | ||
| "/usr/local/include", // Extra system headers | ||
| "/opt/arm-toolchain/include" // Toolchain headers (adjust path) |
There was a problem hiding this comment.
The include path "/opt/arm-toolchain/include" is an absolute path specific to a particular machine setup. This makes the workspace configuration non-portable for other contributors. Consider using environment variables or relative paths to ensure the project can be configured correctly on different systems.
1 participant
Just returns VREF for scaling adc readings.