fix(input): Add i18n, constraints, configurable fault threshold, and bug fixes for ADS1115 pH/EC#28
Conversation
|
@copilot plz remove the word restore from the pr title, before it creats its own life |
De huidige PR titel bevat geen "restore": "fix(input): Add i18n and min-7 constraint for ADS1115 pH/EC oversampling options" De PR beschrijving focust op sampling en spike filtering verbeteringen zonder "restore" te noemen. Als je bedoelt de base branch titel (feature/ads1115-ph-ec-filtering-v2), dan moet die handmatig aangepast worden - ik kan alleen deze sub-PR beheren. |
There was a problem hiding this comment.
Pull request overview
This PR improves the ADS1115 pH/EC input module by adding constraint validation for the minimum sample count required by IQR (Interquartile Range) filtering and adding internationalization support to user-facing strings.
Changes:
- Adds
constraints_pass_oversample_countfunction to enforce minimum 7 samples for IQR filtering - Wraps 'name' and 'phrase' fields in custom_options with
lazy_gettextfor internationalization - Removes hardcoded "Min 7" text from UI since validation is now programmatic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
28f9297 to
6dda84f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| 'type': 'message', | ||
| 'default_value': 'Oversampling', | ||
| }, |
There was a problem hiding this comment.
PR description says the new 'Oversampling' section header was wrapped with lazy_gettext, but the code sets the message default_value to a plain string ('Oversampling'). If the description is meant to reflect the implementation, update either the description or the code (noting other message default_value entries in this file are also plain strings).
| # Verify: read with same method as get_measurement() and check result | ||
| time.sleep(1) | ||
| verify_v = self.get_volt_data(int(self.adc_channel_ph)) | ||
| verify_ph = self.convert_volt_to_ph(verify_v, temp) |
There was a problem hiding this comment.
In calibration verification, convert_volt_to_ph() is called with temp instead of the computed t (which falls back to 25°C when no temp measurement exists). This makes verification inconsistent with the calibration temperature that was just stored and can skew the deviation check when temp is None. Pass t (or re-compute the same fallback) into convert_volt_to_ph() during verification.
| verify_ph = self.convert_volt_to_ph(verify_v, temp) | |
| verify_ph = self.convert_volt_to_ph(verify_v, t) |
| # Verify: read with same method as get_measurement() and check result | ||
| time.sleep(1) | ||
| verify_v = self.get_volt_data(int(self.adc_channel_ec)) | ||
| verify_ec = self.convert_volt_to_ec(verify_v, temp) |
There was a problem hiding this comment.
In EC calibration verification, convert_volt_to_ec() is called with temp instead of the computed fallback t (25°C when no temp measurement exists). This can make the verification result inconsistent with the temperature value saved during calibration. Use t (or the same fallback logic) for the verification conversion.
| verify_ec = self.convert_volt_to_ec(verify_v, temp) | |
| verify_ec = self.convert_volt_to_ec(verify_v, t) |
| if volt > 3.7: | ||
| self.logger.error("pH Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | ||
| self.value_set(0, None) # Return None to prevent erratic control behavior |
There was a problem hiding this comment.
The if volt > 3.7: block uses inconsistent indentation relative to the surrounding code (extra leading spaces), which can raise an IndentationError or make the block visually misleading. Align indentation to the file’s standard (4 spaces per block).
| self.logger.error("pH Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | ||
| self.value_set(0, None) # Return None to prevent erratic control behavior |
There was a problem hiding this comment.
value_set() rejects None (it logs Error 100 and returns early), so calling self.value_set(0, None) won’t actually record a safe/empty value and will add extra error logs. If the intent is to suppress the measurement on fault, avoid calling value_set() and instead leave the channel unset or set an explicit fallback value that is acceptable to value_set().
| self.logger.error("EC Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | ||
| self.value_set(1, None) # Return None to prevent erratic control behavior |
There was a problem hiding this comment.
The EC fault path has the same issue as pH: the if volt > 3.7: branch has inconsistent indentation and calls self.value_set(1, None), but value_set() does not accept None. Fix indentation and avoid calling value_set() with None (leave value unset or use an acceptable sentinel value).
| self.logger.error("EC Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | |
| self.value_set(1, None) # Return None to prevent erratic control behavior | |
| self.logger.error("EC Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) |
|
@copilot apply changes based on the comments in this thread |
m0nk1111
left a comment
m0nk1111
left a comment
There was a problem hiding this comment.
Review: ADS1115 pH/EC Oversampling + IQR Filtering
Overall: Solid feature — the IQR spike rejection and multisample calibration are genuinely useful improvements for the ADS1115. The core algorithm is well-designed. However, there are several issues that need fixing before this is merge-ready, especially if the goal is upstream contribution to kizniche/Mycodo.
🔴 Must Fix (Bugs)
tempvstin verification (lines 472, 536) —convert_volt_to_ph/ec(verify_v, temp)passes rawtemp(possiblyNone) instead of the fallbackt(25°C). Will crash when no temperature sensor is connected.value_set(channel, None)(lines 752, 766) — Mycodo'svalue_set()rejectsNone(Error 100). Should skip callingvalue_set()entirely to suppress the measurement.- Indentation (lines 752-753, 766-767) — Extra leading space will cause inconsistency or errors.
🟡 Should Fix (Upstream Quality)
- Hardcoded 3.7V threshold is DFRobot-specific — "Generic Analog pH/EC" input shouldn't have brand-specific thresholds. Make configurable or phrase generically.
- Error message not i18n (line 31) —
"Must be at least 7 for IQR filtering"should uselazy_gettext(). calibration_samplesallows 1 —constraints_pass_positive_valueis too permissive. Min 5 would be more appropriate.
🟢 Nice to Have (Polish)
- Document timing —
get_volt_data()now takes ~270ms/call,get_volt_data_multisample()~15s. Add brief comments for maintainers. - Magic number 0.002 (2mV IQR floor) — Add inline comment explaining purpose.
- Nested oversampling — Document that calibration does 20×15=300 reads in docstring.
✅ What Works Well
- IQR spike rejection with 2mV floor is clever and well-implemented
- Calibration verification with deviation warnings is a great UX addition
- Using median instead of mean for both measurement and calibration is correct
constraints_pass_oversample_countenforces min-7 programmaticallylazy_gettext()on option names/phrases is consistent with codebase conventions
Merge Conflicts
PR state is dirty — needs rebase on feature/ads1115-ph-ec-filtering-v2 before merge.
| # ADS1115 open input or short to 5V will read >4.0V on gain 1. | ||
| if volt > 3.7: | ||
| self.logger.error("EC Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | ||
| self.value_set(1, None) # Return None to prevent erratic control behavior |
There was a problem hiding this comment.
Same issues as pH sanity check: indentation, value_set(1, None) rejection, DFRobot-specific comment.
if volt > 3.7:
self.logger.error(
"EC Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt))
# Leave value unset to skip this measurement cycle| 'default_value': 20, | ||
| 'constraints_pass': constraints_pass_positive_value, | ||
| 'name': lazy_gettext('Calibration Samples'), | ||
| 'phrase': lazy_gettext('Number of measurements to collect during calibration.') |
There was a problem hiding this comment.
constraints_pass_positive_value allows 1, but calibrating with 1-2 samples is meaningless for quality. statistics.stdev() requires ≥2 values (handled by the fallback, but still).
Suggest minimum of 5 with a custom constraint:
def constraints_pass_calibration_samples(mod_input, value):
errors = []
all_passed = True
if value < 5:
all_passed = False
errors.append(lazy_gettext("Must be at least 5 for reliable calibration"))
return all_passed, errors, mod_input| self.logger.debug( | ||
| "Ch{}: Gain {}, {:.4f}V (median of {})".format( | ||
| channel, self.adc_gain, volt_data, len(readings))) | ||
| return volt_data |
There was a problem hiding this comment.
Nested oversampling: this calls get_volt_data() which itself does 15 oversampled reads. So default calibration = 20 × 15 = 300 ADC reads, ~15 seconds total.
Totally fine for calibration, but worth adding timing info in the docstring so upstream users know what to expect:
"""Collect multiple get_volt_data() readings for calibration.
Uses the same measurement method as get_measurement() to avoid
systematic offset between calibration and measurement.
Note: Default takes ~15s (20 readings × 0.5s interval,
each reading internally oversamples with IQR filtering).
Returns (median_voltage, std_deviation).
"""|
|
||
| readings = [] | ||
| for _ in range(num_samples): | ||
| readings.append(chan.voltage) |
There was a problem hiding this comment.
Performance note: 15 samples × ~18ms (10ms sleep + ~8ms ADC read at 128 SPS) ≈ 270ms per call. get_measurement() calls this twice (pH + EC) = ~540ms minimum per measurement cycle.
For typical periods (15-300s) this is fine, but worth a brief inline comment for maintainers:
# ~18ms per sample (10ms sleep + ~8ms ADC conversion at 128 SPS)
# Default 15 samples ≈ 270ms per channelAlso: at higher gains (8/16) the ADS1115 data rate may be lower. Not blocking, just document.
| q1 = sorted_r[len(sorted_r) // 4] | ||
| q3 = sorted_r[3 * len(sorted_r) // 4] | ||
| iqr = q3 - q1 | ||
| fence = 3.0 * max(iqr, 0.002) # 2mV minimum IQR floor |
There was a problem hiding this comment.
The 0.002 (2mV) magic number needs a comment:
| fence = 3.0 * max(iqr, 0.002) # 2mV minimum IQR floor | |
| fence = 3.0 * max(iqr, 0.002) # 2mV floor: prevents overly tight fences when signal is very stable |
Without the floor, a perfectly stable signal (IQR≈0) would reject everything as a "spike". Good defensive coding — just document it.
| all_passed = False | ||
| errors.append("Must be at least 7 for IQR filtering") | ||
| return all_passed, errors, mod_input | ||
|
|
There was a problem hiding this comment.
Error message is user-facing but not wrapped in lazy_gettext(). For upstream consistency:
| errors.append(lazy_gettext("Must be at least 7 for IQR filtering")) |
| # Verify: read with same method as get_measurement() and check result | ||
| time.sleep(1) | ||
| verify_v = self.get_volt_data(int(self.adc_channel_ec)) | ||
| verify_ec = self.convert_volt_to_ec(verify_v, temp) |
There was a problem hiding this comment.
Same temp vs t bug as pH verification:
| verify_ec = self.convert_volt_to_ec(verify_v, temp) | |
| verify_ec = self.convert_volt_to_ec(verify_v, t) |
| # Isolator might push slightly higher, but >3.7V is almost certainly a fault. | ||
| if volt > 3.7: | ||
| self.logger.error("pH Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt)) | ||
| self.value_set(0, None) # Return None to prevent erratic control behavior |
There was a problem hiding this comment.
1. DFRobot-specific logic in a "Generic" input
The comments reference "DFRobot pH 2.0" and "DFRobot EC max output". For upstream to kizniche/Mycodo, a "Generic Analog pH/EC" input shouldn't hardcode brand-specific thresholds. Either:
- Make
voltage_fault_thresholda configurable custom option (default 3.7) - Or at minimum rephrase comments generically
2. value_set(0, None) won't work
As the auto-reviewer noted, Mycodo's value_set() rejects None (logs Error 100). Simply don't call value_set() — leaving it unset effectively skips the measurement. Same for EC below.
3. Indentation
Extra space before self.logger.error and self.value_set.
Suggested fix:
if volt > 3.7:
self.logger.error(
"pH Voltage {:.4f}V > 3.7V! Possible disconnect or hardware fault.".format(volt))
# Leave value unset to skip this measurement cycle| # Verify: read with same method as get_measurement() and check result | ||
| time.sleep(1) | ||
| verify_v = self.get_volt_data(int(self.adc_channel_ph)) | ||
| verify_ph = self.convert_volt_to_ph(verify_v, temp) |
There was a problem hiding this comment.
🐛 Bug: temp vs t — when self.get_temp_data() returns None, temp is None but t is 25. Verification then calls convert_volt_to_ph(verify_v, None) which will crash or produce garbage. The auto-reviewer caught this too.
| verify_ph = self.convert_volt_to_ph(verify_v, temp) | |
| verify_ph = self.convert_volt_to_ph(verify_v, t) |
All review comments addressed in commits 9ca89d1 and 5b165ed: ✅ pH/EC calibration verification now uses temperature fallback Code review: no issues | CodeQL: 0 alerts |
All review feedback addressed in commits bfb3629 and defa501: 🔴 Must Fix (Bugs) - Already fixed in 9ca89d1, 5b165ed: 🟡 Should Fix (Upstream Quality): 🟢 Nice to Have (Polish): Note: DFRobot 3.7V threshold remains hardcoded with clarifying comments. Making it configurable would be a good future enhancement but out of scope for this i18n/constraint PR. Code review: clean | CodeQL: 0 alerts |
…bug fixes for ADS1115 pH/EC Changes on top of the base branch (preserves 3-point EC calibration): Constraint enforcement: - Add constraints_pass_oversample_count (min 7 for IQR filtering) - Add constraints_pass_calibration_samples (min 5 for reliability) - Replace generic constraints_pass_positive_value on both options Internationalization: - Wrap oversample_count name/phrase with lazy_gettext - Wrap calibration_samples name/phrase with lazy_gettext - Wrap voltage_fault_threshold name/phrase with lazy_gettext Bug fixes: - Fix temp vs t in pH calibration verification (would crash when no temperature sensor is connected) - Fix temp vs t in EC calibration verification (same issue) - Remove value_set(channel, None) calls (Mycodo rejects None) - Fix indentation in sanity check blocks Configurable voltage fault threshold: - Add voltage_fault_threshold custom option (default 3.7V) - Replace hardcoded 3.7V in get_measurement() sanity checks - Remove DFRobot-specific comments (this is a Generic input) Documentation: - Add performance timing to get_volt_data() docstring - Add nested oversampling timing to get_volt_data_multisample() - Document 2mV IQR floor purpose in inline comment
defa501 to
5ad1101
Compare
m0nk1111
changed the title
Applies review fixes and improvements on top of the
feature/ads1115-ph-ec-filtering-v2base branch. Preserves all existing features (3-point EC calibration, IQR filtering, multisample calibration).Changes
Constraint enforcement
constraints_pass_oversample_countenforcing minimum of 7 samples (required for median + IQR filtering)constraints_pass_calibration_samplesenforcing minimum of 5 samples (required for reliable calibration)constraints_pass_positive_valueon both optionsInternationalization
oversample_count,calibration_samples, andvoltage_fault_thresholdoption names/phrases withlazy_gettext'Oversampling'remains a plain string per codebase convention (messagedefault_valuefields are not translated)Bug fixes
tempvstin pH verification:convert_volt_to_ph(verify_v, temp)→convert_volt_to_ph(verify_v, t)— was passingNonewhen no temperature sensor is connected, causing crashtempvstin EC verification: Same fix for EC calibration verificationvalue_set(channel, None): Removed — Mycodo'svalue_set()rejectsNone(Error 100). Measurement is now left unset to skip the cycle.Configurable voltage fault threshold
voltage_fault_thresholdcustom option (default 3.7V)3.7inget_measurement()sanity checks with configurable valueDocumentation
get_volt_data()docstring (~270ms per channel)get_volt_data_multisample()docstring (~15s, 300 ADC reads)