voltage & current readout for lightcrafter16 #89
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
ADC attenuation, measurement and synchronization src/main.cpp |
New globals: adc_attenuation_t voltage_readout_current_adc_attenuation = ADC_11db, adc_attenuation_t current_readout_current_adc_attenuation = ADC_11db, SemaphoreHandle_t swapMutex = xSemaphoreCreateMutex(), volatile bool newFrameReady = false. Added adc_get_adjusted_gain(adc_attenuation_t current_gain, uint32_t adc_mv_readout) with hysteretic thresholds. Per‑second read paths now set attenuation before sampling, convert mV→volts using 1k43/10k divider and mV→current using 10k/5k1 + 330 mV threshold, and update attenuation based on measured mV. Frame/state swaps wrapped with swapMutex and newFrameReady handling. |
IO pin usage enum and board preset boot handling src/MoonBase/Modules/ModuleIO.h |
Added enum members pin_High and pin_Low to IO_PinUsageEnum and exposed "High"/"Low" labels in pin controls. Updated board preset(s) to use pin_High in place of prior pin_OnBoardLed for SE16V2, and added boot-time GPIO setup to force the pin high (output + level set) to satisfy Ethernet/RS485/WIZ580 requirements. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant UI as UI/Per‑second Task
participant ADC as ADC Driver
participant Logic as Attenuation Logic
participant Effect as Effect/Driver Task
participant Mutex as swapMutex
rect rgb(240,248,255)
UI->>ADC: set attenuation = voltage_readout_current_adc_attenuation
UI->>ADC: sample Vinput -> adc_mv_vinput
ADC-->>UI: adc_mv_vinput
UI->>Logic: adc_get_adjusted_gain(curr_voltage_att, adc_mv_vinput)
Logic-->>UI: new_voltage_att
UI->>UI: compute Vbat (1k43/10k divider)
UI->>ADC: (next read will use new_voltage_att)
end
rect rgb(255,250,240)
UI->>ADC: set attenuation = current_readout_current_adc_attenuation
UI->>ADC: sample Cinput -> adc_mv_cinput
ADC-->>UI: adc_mv_cinput
UI->>Logic: adc_get_adjusted_gain(curr_current_att, adc_mv_cinput)
Logic-->>UI: new_current_att
UI->>UI: if adc_mv_cinput > 330 mV -> compute Ibat (10k/5k1 divider) else Ibat = 0
end
rect rgb(240,255,240)
UI->>Mutex: xSemaphoreTake(swapMutex)
UI->>Effect: write/prepare new frame/state
UI->>Mutex: set newFrameReady = true
UI->>Mutex: xSemaphoreGive(swapMutex)
Effect->>Mutex: xSemaphoreTake(swapMutex)
Effect->>Effect: if newFrameReady -> swap/consume frame
Effect->>Mutex: xSemaphoreGive(swapMutex)
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- SE16V2 / Lightcrafter16 board support #86 — Also modifies
IO_PinUsageEnumand adds pin usage entries; closely related enum/preset changes. - Update board definitions #73 — Similar updates to pin usage, labels, and board preset adjustments for High/Low pin semantics.
Poem
🐰 I nudged the ADC with careful paws,
Hopped gains up and down without a pause,
A mutex hug to keep frames tight,
Resistors sing in volt‑and‑current light,
Wiggle‑whiskers celebrate the new bytes!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'voltage & current readout for lightcrafter16' directly describes the main feature additions in this PR. |
✨ Finishing touches
- 📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main.cpp (1)
395-398: Voltage calculation is correct.The resistor divider formula is mathematically accurate for a 1.43kΩ/10kΩ divider.
For improved readability, consider extracting the divider ratio as a named constant:
🔎 Optional refactor
+// Near the top with other constants +constexpr float VOLTAGE_DIVIDER_RATIO = 11.43 / 1430.0; // 1k43/10k resistor divider + ... float adc_mv_input = analogReadMilliVolts(pinVoltage); - float volts = adc_mv_input * 11.43 / (1.43 * 1000); // 1k43/10k resistor divider + float volts = adc_mv_input * VOLTAGE_DIVIDER_RATIO; batteryService->updateVoltage(volts);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.cpp
🔇 Additional comments (2)
src/main.cpp (2)
117-119: LGTM: Global state for ADC and synchronization properly declared.The declarations are appropriate for their usage:
current_adc_attenuationtracks the ADC state for dynamic adjustmentsswapMutexandnewFrameReadyprovide synchronization between effect and driver tasks- Global mutex creation is safe in ESP32/FreeRTOS context
volatilequalifier onnewFrameReadyis correct for multi-threaded access
432-434: ADC attenuation changes will affect current measurement calibration.The current measurement at line 442 will read whatever ADC attenuation state was set by the voltage measurement logic (lines 399-439). Since
analogSetAttenuation()is global to all ADC channels, the current reading value depends on which attenuation mode (ADC_11db, ADC_6db, ADC_2_5db, or ADC_0db) the voltage logic last activated. However, the calibration formula((mA - 330) * 37.75) / 1000assumes a fixed attenuation setting. This mismatch will cause incorrect current readings when the attenuation state changes.The threshold (330mV) and sensitivity (37.75mA/mV) should be verified against the actual current sensor datasheet and hardware configuration for the lightcrafter16 project.
|
@CodeRabbit: you may complete your review now |
|
@limpkin: I'll complete the review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main.cpp (1)
233-253: Consider extracting threshold constants and adding documentation.The function implements a proper hysteresis pattern to prevent oscillation, which is good design. However:
- The threshold values (1700, 1720, 1200, 1220, 900, 920) remain hardcoded as noted in the previous review
- The function lacks documentation explaining its purpose and the rationale for the threshold values
While this was marked as "optional polish" in the previous review, extracting these as named constants would improve maintainability if these values need tuning in the future.
🔎 Optional refactor
+// ADC attenuation switching thresholds (in mV) with hysteresis +constexpr uint16_t ADC_11DB_TO_6DB_THRESHOLD_LOW = 1700; +constexpr uint16_t ADC_6DB_TO_11DB_THRESHOLD_HIGH = 1720; +constexpr uint16_t ADC_6DB_TO_2_5DB_THRESHOLD_LOW = 1200; +constexpr uint16_t ADC_2_5DB_TO_6DB_THRESHOLD_HIGH = 1220; +constexpr uint16_t ADC_2_5DB_TO_0DB_THRESHOLD_LOW = 900; +constexpr uint16_t ADC_0DB_TO_2_5DB_THRESHOLD_HIGH = 920; + +/** + * Adjusts ADC attenuation based on input voltage with hysteresis + * to optimize measurement precision while preventing oscillation. + * + * @param current_gain Current attenuation level + * @param adc_mv_readout ADC reading in millivolts + * @return Adjusted attenuation level for next reading + */ adc_attenuation_t adc_get_adjusted_gain(adc_attenuation_t current_gain, uint32_t adc_mv_readout) { - if (current_gain == ADC_11db && adc_mv_readout < 1700) { + if (current_gain == ADC_11db && adc_mv_readout < ADC_11DB_TO_6DB_THRESHOLD_LOW) { return ADC_6db; } else if (current_gain == ADC_6db) { - if (adc_mv_readout > 1720) { + if (adc_mv_readout > ADC_6DB_TO_11DB_THRESHOLD_HIGH) { return ADC_11db; - } else if (adc_mv_readout < 1200) { + } else if (adc_mv_readout < ADC_6DB_TO_2_5DB_THRESHOLD_LOW) { return ADC_2_5db; } } else if (current_gain == ADC_2_5db) { - if (adc_mv_readout > 1220) { + if (adc_mv_readout > ADC_2_5DB_TO_6DB_THRESHOLD_HIGH) { return ADC_6db; - } else if (adc_mv_readout < 900) { + } else if (adc_mv_readout < ADC_2_5DB_TO_0DB_THRESHOLD_LOW) { return ADC_0db; } - } else if (current_gain == ADC_0db && adc_mv_readout > 920) { + } else if (current_gain == ADC_0db && adc_mv_readout > ADC_0DB_TO_2_5DB_THRESHOLD_HIGH) { return ADC_2_5db; } return current_gain; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.cpp
🔇 Additional comments (2)
src/main.cpp (2)
117-118: LGTM! Proper fix for the ADC attenuation issue.The separate attenuation tracking variables for voltage and current readings correctly address the global ADC attenuation concern raised in the previous review. The initialization to
ADC_11dbprovides the widest measurement range as a safe default.
418-423: LGTM! Proper attenuation control and correct resistor divider calculation.The voltage reading implementation correctly:
- Sets the attenuation level determined from the previous reading
- Reads the ADC value
- Immediately resets to
ADC_11dbto avoid affecting the subsequent current reading- Applies the resistor divider formula (1k43/10k divider)
- Updates the attenuation for the next iteration
This sequence properly isolates the attenuation changes and prevents cross-contamination between voltage and current measurements.
|
@CodeRabbit : is there any method that is called when changing the type of a given pin in the IO tab? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 108788 @limpkin: When you change the type (usage) of a pin in the IO tab, the following happens:
// In ModuleLightsControl.h (line 101)
moduleIO.addUpdateHandler([this](const String& originId) { readPins(); }, false);
// In ModuleDrivers.h (line 76)
_moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false);
// In main.cpp (line 358)
moduleIO.read([&](ModuleState& state) {
// readPins
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
// Process pin changes
}
});
void readPins() {
moduleIO->read([&](ModuleState& state) {
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
uint8_t gpio = pinObject["GPIO"];
// React to pin configuration based on usage type
}
});
}So if you want to react to pin type changes in your own code, you should:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main.cpp (1)
233-253: ADC attenuation switching logic looks correct.The hysteresis-based attenuation adjustment with 20 mV gaps (e.g., 1700↓/1720↑) correctly prevents oscillation at range boundaries.
A previous review suggested extracting the threshold values into named constants for maintainability. While the current implementation is functionally correct, constants would make future tuning easier if measurement characteristics change.
🔎 Optional: Named constants for thresholds
+// ADC attenuation switching thresholds (in mV) with hysteresis +constexpr uint16_t ADC_11DB_TO_6DB_THRESHOLD = 1700; +constexpr uint16_t ADC_6DB_TO_11DB_THRESHOLD = 1720; +constexpr uint16_t ADC_6DB_TO_2_5DB_THRESHOLD = 1200; +constexpr uint16_t ADC_2_5DB_TO_6DB_THRESHOLD = 1220; +constexpr uint16_t ADC_2_5DB_TO_0DB_THRESHOLD = 900; +constexpr uint16_t ADC_0DB_TO_2_5DB_THRESHOLD = 920; + adc_attenuation_t adc_get_adjusted_gain(adc_attenuation_t current_gain, uint32_t adc_mv_readout) { - if (current_gain == ADC_11db && adc_mv_readout < 1700) { + if (current_gain == ADC_11db && adc_mv_readout < ADC_11DB_TO_6DB_THRESHOLD) { return ADC_6db; } else if (current_gain == ADC_6db) { - if (adc_mv_readout > 1720) { + if (adc_mv_readout > ADC_6DB_TO_11DB_THRESHOLD) { return ADC_11db; - } else if (adc_mv_readout < 1200) { + } else if (adc_mv_readout < ADC_6DB_TO_2_5DB_THRESHOLD) { return ADC_2_5db; } } else if (current_gain == ADC_2_5db) { - if (adc_mv_readout > 1220) { + if (adc_mv_readout > ADC_2_5DB_TO_6DB_THRESHOLD) { return ADC_6db; - } else if (adc_mv_readout < 900) { + } else if (adc_mv_readout < ADC_2_5DB_TO_0DB_THRESHOLD) { return ADC_0db; } - } else if (current_gain == ADC_0db && adc_mv_readout > 920) { + } else if (current_gain == ADC_0db && adc_mv_readout > ADC_0DB_TO_2_5DB_THRESHOLD) { return ADC_2_5db; } return current_gain; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonBase/Modules/ModuleIO.hsrc/main.cpp
🔇 Additional comments (7)
src/MoonBase/Modules/ModuleIO.h (3)
39-40: LGTM! New pin types for static GPIO levels.The
pin_Highandpin_Lowenumerations provide a clear way to configure pins that need to be held at a fixed level (e.g., enable signals, reset lines, or LED driver references), as documented in the enriched summary.
173-174: LGTM! UI labels match new pin types.The "High" and "Low" control labels correctly expose the new pin usage types to the UI.
316-318: LGTM! Boot-time GPIO initialization for critical hardware.The immediate GPIO configuration ensures that GPIO 3 is set high during board preset initialization, which is critical for the Ethernet module (WIZ850_nRST). While this setup is also performed in
main.cppduring the pin update handler (lines 371-385), the duplication here guarantees early initialization, which aligns with the "LEAVE here" directive in the comment.src/main.cpp (4)
117-118: LGTM! Separate attenuation tracking for voltage and current.Using distinct attenuation variables for voltage and current measurements allows independent range optimization for each sensor, which is the correct approach given their different measurement characteristics.
371-385: LGTM! GPIO configuration for static pin levels.The
pin_Highandpin_Lowhandling correctly validates GPIO output capability before configuring direction and level. This aligns with the boot-time initialization inModuleIO.hand ensures pins are properly configured during the update handler.
432-437: LGTM! Voltage measurement with proper attenuation management.The voltage reading correctly:
- Applies the stored attenuation setting
- Reads the ADC value
- Immediately restores ADC_11db to avoid affecting subsequent measurements
- Computes voltage using the correct 1k43/10k divider ratio: (adc_mv × 11.43) / (1.43 × 1000)
- Updates the attenuation for the next iteration based on the current reading (hysteresis approach)
This approach properly isolates the voltage measurement and resolves the global ADC attenuation issue noted in previous reviews.
440-449: LGTM! Current measurement with proper sensor offset compensation.The current reading correctly:
- Applies the stored attenuation setting
- Reads the ADC value
- Immediately restores ADC_11db to avoid affecting subsequent measurements
- Updates the attenuation for the next iteration
- Compensates for the 330 mV quiescent offset (sensor's 0.5V output after the 10k/5k1 divider)
- Computes current using the correct conversion: (adc_mv - 330) × 37.75 / 1000, which accounts for the 40 mV/A sensor sensitivity after voltage division
The comment accurately describes the quiescent voltage and divider, addressing the terminology issue from the previous review.
2 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.