wip: adc_ll refactor + dmm #96
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR revamps the ADC low-level driver by introducing dynamic channel and oversampling support, refactors platform clock setup to provide a dedicated 75 MHz ADC clock, replaces direct ADC usage in the application with a new Digital Multimeter (DMM) instrument module, and adds a fixed-point library and comprehensive DMM unit tests along with build script updates. Class diagram for the new Digital Multimeter (DMM) instrumentclassDiagram
class DMM_Handle {
+DMM_Config config
+uint16_t adc_buffer
+volatile bool conversion_complete
+bool initialized
}
class DMM_Config {
+DMM_Channel channel
+uint32_t oversampling_ratio
+uint32_t timeout_ms
+Fixed reference_voltage
}
class DMM_Channel {
<<enum>>
DMM_CHANNEL_0
DMM_CHANNEL_1
DMM_CHANNEL_2
DMM_CHANNEL_3
DMM_CHANNEL_4
DMM_CHANNEL_5
DMM_CHANNEL_6
DMM_CHANNEL_7
DMM_CHANNEL_8
DMM_CHANNEL_9
DMM_CHANNEL_10
DMM_CHANNEL_11
DMM_CHANNEL_12
DMM_CHANNEL_13
DMM_CHANNEL_14
DMM_CHANNEL_15
}
class Fixed {
<<typedef>>
int32_t (Q16.16 fixed-point)
}
DMM_Handle --> DMM_Config
DMM_Config --> DMM_Channel
DMM_Config --> Fixed
DMM_Handle : +DMM_init(config)
DMM_Handle : +DMM_deinit(handle)
DMM_Handle : +DMM_read_voltage(handle, voltage_out)
Class diagram for the refactored ADC_LL driverclassDiagram
class ADC_LL_Config {
+ADC_LL_Channel channel
+ADC_LL_TriggerSource trigger_source
+uint16_t* output_buffer
+uint32_t buffer_size
+uint32_t oversampling_ratio
}
class ADC_LL_Channel {
<<enum>>
ADC_LL_CHANNEL_0
ADC_LL_CHANNEL_1
ADC_LL_CHANNEL_2
ADC_LL_CHANNEL_3
ADC_LL_CHANNEL_4
ADC_LL_CHANNEL_5
ADC_LL_CHANNEL_6
ADC_LL_CHANNEL_7
ADC_LL_CHANNEL_8
ADC_LL_CHANNEL_9
ADC_LL_CHANNEL_10
ADC_LL_CHANNEL_11
ADC_LL_CHANNEL_12
ADC_LL_CHANNEL_13
ADC_LL_CHANNEL_14
ADC_LL_CHANNEL_15
}
class ADC_LL_TriggerSource {
<<enum>>
ADC_TRIGGER_TIMER1
ADC_TRIGGER_TIMER1_TRGO2
ADC_TRIGGER_TIMER2
ADC_TRIGGER_TIMER3
ADC_TRIGGER_TIMER4
ADC_TRIGGER_TIMER6
ADC_TRIGGER_TIMER8
ADC_TRIGGER_TIMER8_TRGO2
ADC_TRIGGER_TIMER15
}
ADC_LL_Config --> ADC_LL_Channel
ADC_LL_Config --> ADC_LL_TriggerSource
class ADCInstance {
+ADC_HandleTypeDef* adc_handle
+DMA_HandleTypeDef* dma_handle
+ADC_ChannelConfTypeDef adc_config
+uint16_t* adc_buffer_data
+uint32_t adc_buffer_size
+ADC_LL_CompleteCallback adc_complete_callback
+ADC_LL_Channel current_channel
+uint32_t oversampling_ratio
+bool initialized
}
ADCInstance --> ADC_LL_Channel
Class diagram for the new fixed-point libraryclassDiagram
class Fixed {
<<typedef>>
int32_t (Q16.16 fixed-point)
}
Fixed : +FIXED_FROM_INT(x)
Fixed : +FIXED_TO_INT(x)
Fixed : +FIXED_FROM_FLOAT(f)
Fixed : +FIXED_TO_FLOAT(x)
Fixed : +fixed_add(a, b)
Fixed : +fixed_sub(a, b)
Fixed : +fixed_mul(a, b)
Fixed : +fixed_div(a, b)
Fixed : +fixed_mul_int(a, b)
Fixed : +fixed_div_int(a, b)
Fixed : +fixed_from_fraction(num, den)
Fixed : +fixed_from_millivolts(mv)
Fixed : +fixed_to_millivolts(volts)
Fixed : +fixed_get_fractional_part(x)
Fixed : +fixed_get_integer_part(x)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bessman - I've reviewed your changes - here's some feedback:
- The repetitive channel-to-pin, channel-to-HAL constant, and trigger mappings could be consolidated into static lookup tables or helper macros to reduce boilerplate and simplify future updates.
- ADC_LL_get_sample_rate currently does multiple runtime lookups—consider computing and caching the sample rate once during init to avoid the overhead of rebuilding lookup tables on each call.
- The DMM read_raw function uses a busy-wait loop for conversion completion, which ties up the CPU—consider leveraging interrupt-driven synchronization or a hardware timer/semaphore instead of active polling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repetitive channel-to-pin, channel-to-HAL constant, and trigger mappings could be consolidated into static lookup tables or helper macros to reduce boilerplate and simplify future updates.
- ADC_LL_get_sample_rate currently does multiple runtime lookups—consider computing and caching the sample rate once during init to avoid the overhead of rebuilding lookup tables on each call.
- The DMM read_raw function uses a busy-wait loop for conversion completion, which ties up the CPU—consider leveraging interrupt-driven synchronization or a hardware timer/semaphore instead of active polling.
## Individual Comments
### Comment 1
<location> `src/platform/h563xx/adc_ll.c:312` </location>
<code_context>
}
+ // Validate oversampling ratio
+ if (config->oversampling_ratio != 1 && config->oversampling_ratio != 2 &&
+ config->oversampling_ratio != 4 && config->oversampling_ratio != 8 &&
+ config->oversampling_ratio != 16 && config->oversampling_ratio != 32 &&
+ config->oversampling_ratio != 64 && config->oversampling_ratio != 128 &&
+ config->oversampling_ratio != 256) {
+ THROW(ERROR_INVALID_ARGUMENT);
+ return;
</code_context>
<issue_to_address>
Oversampling ratio validation is unnecessarily verbose.
Consider replacing the explicit checks with a single condition that verifies the value is a power of two between 1 and 256. This will improve maintainability and reduce potential errors when supporting more ratios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Validate oversampling ratio
if (config->oversampling_ratio != 1 && config->oversampling_ratio != 2 &&
config->oversampling_ratio != 4 && config->oversampling_ratio != 8 &&
config->oversampling_ratio != 16 && config->oversampling_ratio != 32 &&
config->oversampling_ratio != 64 && config->oversampling_ratio != 128 &&
config->oversampling_ratio != 256) {
THROW(ERROR_INVALID_ARGUMENT);
return;
=======
// Validate oversampling ratio: must be a power of two between 1 and 256
if (config->oversampling_ratio == 0 ||
config->oversampling_ratio > 256 ||
(config->oversampling_ratio & (config->oversampling_ratio - 1)) != 0) {
THROW(ERROR_INVALID_ARGUMENT);
return;
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/platform/h563xx/adc_ll.c
Outdated
Comment on lines
311
to
318
| // Validate oversampling ratio | ||
| if (config->oversampling_ratio != 1 && config->oversampling_ratio != 2 && | ||
| config->oversampling_ratio != 4 && config->oversampling_ratio != 8 && | ||
| config->oversampling_ratio != 16 && config->oversampling_ratio != 32 && | ||
| config->oversampling_ratio != 64 && config->oversampling_ratio != 128 && | ||
| config->oversampling_ratio != 256) { | ||
| THROW(ERROR_INVALID_ARGUMENT); | ||
| return; |
There was a problem hiding this comment.
suggestion: Oversampling ratio validation is unnecessarily verbose.
Consider replacing the explicit checks with a single condition that verifies the value is a power of two between 1 and 256. This will improve maintainability and reduce potential errors when supporting more ratios.
Suggested change
| // Validate oversampling ratio | |
| if (config->oversampling_ratio != 1 && config->oversampling_ratio != 2 && | |
| config->oversampling_ratio != 4 && config->oversampling_ratio != 8 && | |
| config->oversampling_ratio != 16 && config->oversampling_ratio != 32 && | |
| config->oversampling_ratio != 64 && config->oversampling_ratio != 128 && | |
| config->oversampling_ratio != 256) { | |
| THROW(ERROR_INVALID_ARGUMENT); | |
| return; | |
| // Validate oversampling ratio: must be a power of two between 1 and 256 | |
| if (config->oversampling_ratio == 0 || | |
| config->oversampling_ratio > 256 || | |
| (config->oversampling_ratio & (config->oversampling_ratio - 1)) != 0) { | |
| THROW(ERROR_INVALID_ARGUMENT); | |
| return; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a work in progress to refactor the ADC driver and add a basic multimeter instrument.
Summary by Sourcery
Refactor the low-level ADC driver for flexible channel configuration, oversampling, and sample-rate queries, configure the platform to provide a dedicated ADC clock, introduce a new Digital Multimeter (DMM) instrument module with fixed-point support, and update the application to use DMM in place of legacy ADC logic.
New Features:
Enhancements:
Tests: