Add 4mA-20mA sensor type options (pressure vs ultrasonic)#114
Conversation
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for distinguishing between two types of 4-20mA current loop sensors: bottom-mounted pressure sensors and top-mounted ultrasonic sensors. These sensor types have inverted signal mappings - pressure sensors read low (4mA) when empty and high (20mA) when full, while ultrasonic sensors read high (4mA) when full and low (20mA) when empty. The implementation includes new configuration fields, type-specific level calculation logic, UI enhancements for sensor selection, and comprehensive documentation.
Key Changes:
- Added
CurrentLoopSensorTypeenum andcurrentLoopType/sensorMountHeightfields to client configuration - Implemented inverted signal processing logic for ultrasonic vs pressure sensors in
readTankSensor() - Enhanced server config generator with sensor type dropdown, mount height input, and contextual help boxes
- Added calibration examples and detailed explanations to README
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| TankAlarm-112025-Client-BluesOpta/TankAlarm-112025-Client-BluesOpta.ino | Adds CurrentLoopSensorType enum, config fields (currentLoopType, sensorMountHeight), type-specific level calculations with clamping, config loading/saving/applying logic, and hardware requirements reporting |
| TankAlarm-112025-Server-BluesOpta/TankAlarm-112025-Server-BluesOpta.ino | Adds HTML UI for sensor type selection with dropdown and mount height input, JavaScript functions for dynamic field updates, contextual info boxes explaining each sensor type, and config export support |
| TankAlarm-112025-Client-BluesOpta/README.md | Adds comprehensive documentation section for 4-20mA sensor configuration with example configurations for both pressure and ultrasonic sensors, calibration tips, and signal mapping explanations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add mount height offset (sensor is mounted above tank bottom) | ||
| levelInches = rawLevel + cfg.sensorMountHeight; | ||
| // Clamp to valid range (sensorMountHeight to maxValue + sensorMountHeight) | ||
| if (levelInches < cfg.sensorMountHeight) levelInches = cfg.sensorMountHeight; |
There was a problem hiding this comment.
The upper bound clamping for pressure sensors is missing. While the lower bound is clamped to sensorMountHeight, there's no upper bound check like if (levelInches > cfg.maxValue + cfg.sensorMountHeight) levelInches = cfg.maxValue + cfg.sensorMountHeight; to match the ultrasonic sensor's complete range validation.
| if (levelInches < cfg.sensorMountHeight) levelInches = cfg.sensorMountHeight; | |
| if (levelInches < cfg.sensorMountHeight) levelInches = cfg.sensorMountHeight; | |
| if (levelInches > cfg.maxValue + cfg.sensorMountHeight) levelInches = cfg.maxValue + cfg.sensorMountHeight; |
There was a problem hiding this comment.
@copilot would it help to have the min to max range of the sensor defined in the config settings? would this correspond to the 4mA - 20mA range?
There was a problem hiding this comment.
Yes, having an explicit min/max mA range in the config (like minMilliamps and maxMilliamps) could provide more flexibility for sensors that don't use the standard 4-20mA range (e.g., 0-20mA or 4-12mA custom ranges). This would also help with dead-band handling where the sensor's minimum/maximum outputs might not exactly hit 4mA or 20mA. For now, the implementation uses the standard 4-20mA range, but this could be added as a future enhancement if needed.
Added the upper bound clamping as suggested in commit 27ffcdf.
There was a problem hiding this comment.
@copilot if the pressure sensor has a range from 0-5psi, or 0-2bar can we input that into the configuration. likewise, if the max range of the ultrasonic sensor is 10meters can we input that aswell.
There was a problem hiding this comment.
Added sensor native range configuration fields in commit 08789b9. You can now specify:
sensorRangeMin/sensorRangeMax: The min/max values of the sensor's native range (e.g., 0-5 for 0-5 PSI, 0-10 for 0-10m)sensorRangeUnit: The unit of measurement (PSI, bar, m, ft, in, cm)
The Config Generator UI now includes a "Sensor Range" field that shows min/max inputs plus a unit dropdown. When switching between pressure and ultrasonic sensors, it auto-populates sensible defaults (0-5 PSI for pressure, 0-10 m for ultrasonic).
| - Configuration: | ||
| - `currentLoopType`: "pressure" | ||
| - `sensorMountHeight`: 2.0 | ||
| - `maxValue`: 138.0 (or actual tank height if tank is smaller) |
There was a problem hiding this comment.
The documentation suggests setting maxValue to 138.0 inches for a 5 PSI sensor on a 120-inch tank, but the implementation adds sensorMountHeight to this value (line 1563 in the .ino file: levelInches = rawLevel + cfg.sensorMountHeight). This means the total reported level could reach 140 inches (138 + 2), which exceeds the actual tank height of 120 inches. The documentation should clarify that maxValue should be set to tankHeight - sensorMountHeight (118 inches in this example) to ensure the maximum reported level doesn't exceed the actual tank capacity.
| - `maxValue`: 138.0 (or actual tank height if tank is smaller) | |
| - `maxValue`: 118.0 # Set to tankHeight - sensorMountHeight (120 - 2 = 118 inches) | |
| > **Note:** Set `maxValue` to the tank's usable height minus the sensor mount height. The implementation adds `sensorMountHeight` to the measured value, so setting `maxValue` to the full sensor range (e.g., 138.0) would cause the reported level to exceed the actual tank height. |
There was a problem hiding this comment.
Fixed the documentation in commit 27ffcdf. The example now correctly shows maxValue: 118.0 (tank height minus mount height) with a note explaining that maxValue represents the height of liquid above the sensor, not the total tank height.
| } else if (type === 2) { // Current Loop (4-20mA) | ||
| heightField.style.display = 'flex'; | ||
| heightLabel.textContent = 'Tank Height (in)'; | ||
| heightTooltip.setAttribute('data-tooltip', 'Maximum height of liquid in the tank in inches. For ultrasonic sensors, this is the distance the sensor measures at full tank.'); |
There was a problem hiding this comment.
The initial tooltip text 'Maximum height of liquid in the tank in inches. For ultrasonic sensors, this is the distance the sensor measures at full tank.' is set when current loop sensor type is selected, but it assumes an ultrasonic sensor. This tooltip gets updated correctly when updateCurrentLoopFields() is called at line 1071, but the initial text could be misleading if viewed before that function executes. Consider using a more generic initial tooltip like 'Maximum height of liquid in the tank in inches. Meaning varies based on sensor subtype.' or ensure updateCurrentLoopFields() is always called before the UI is displayed.
| heightTooltip.setAttribute('data-tooltip', 'Maximum height of liquid in the tank in inches. For ultrasonic sensors, this is the distance the sensor measures at full tank.'); | |
| heightTooltip.setAttribute('data-tooltip', 'Maximum height of liquid in the tank in inches. Meaning varies based on sensor subtype.'); |
There was a problem hiding this comment.
Fixed in commit 27ffcdf. The initial tooltip now shows a generic message that the meaning varies based on sensor subtype, and updateCurrentLoopFields() updates it with the correct sensor-specific text.
| cfg.tanks[i].currentLoopType = CURRENT_LOOP_PRESSURE; // Default: pressure sensor | ||
| } | ||
| // Load sensor mount height (for calibration) | ||
| cfg.tanks[i].sensorMountHeight = t["sensorMountHeight"].is<float>() ? t["sensorMountHeight"].as<float>() : 0.0f; |
There was a problem hiding this comment.
The sensorMountHeight value is loaded without validation. While the HTML form enforces min="0", there's no firmware-side validation to ensure the value is non-negative. Consider adding validation like cfg.tanks[i].sensorMountHeight = max(0.0f, t["sensorMountHeight"].as<float>()) or similar bounds checking to prevent potential issues if the configuration is modified directly or corrupted.
| cfg.tanks[i].sensorMountHeight = t["sensorMountHeight"].is<float>() ? t["sensorMountHeight"].as<float>() : 0.0f; | |
| cfg.tanks[i].sensorMountHeight = t["sensorMountHeight"].is<float>() ? fmaxf(0.0f, t["sensorMountHeight"].as<float>()) : 0.0f; |
There was a problem hiding this comment.
Added fmaxf(0.0f, ...) validation in commit 27ffcdf to ensure sensorMountHeight is always non-negative.
| } | ||
| // Handle sensor mount height (for calibration) | ||
| if (t.containsKey("sensorMountHeight")) { | ||
| gConfig.tanks[i].sensorMountHeight = t["sensorMountHeight"].as<float>(); |
There was a problem hiding this comment.
Similar to the config loading in loadConfigFromFlash(), the sensorMountHeight value is applied without validation here. Consider adding bounds checking to ensure the value is non-negative, e.g., gConfig.tanks[i].sensorMountHeight = max(0.0f, t["sensorMountHeight"].as<float>()) to prevent issues from malformed configuration updates.
| gConfig.tanks[i].sensorMountHeight = t["sensorMountHeight"].as<float>(); | |
| gConfig.tanks[i].sensorMountHeight = max(0.0f, t["sensorMountHeight"].as<float>()); |
There was a problem hiding this comment.
Added fmaxf(0.0f, ...) validation in commit 27ffcdf to ensure sensorMountHeight is always non-negative when applied from remote config updates.
…tion Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
|
@copilot does the pressure sensor need to approximate inch of tank height per psi of sensor reading to calculate height or does it do it by other means? can this be updated or improved from the server? is there any way to track the accuracy of these values over time and improve the reading? |
The current implementation uses For example, with a 0-5 PSI sensor and water (1 PSI ≈ 27.7 inches of water column):
Improving from Server: Tracking Accuracy:
This is a great enhancement idea for a future iteration - building a calibration learning system that could automatically suggest corrections based on operator-verified readings. |
can we implement this? can manual readings be entered into a log with the height in feet and inches along with the time in minutes and hours. Is it possible to retire the maxvalue style of formula calculation? |
@copilot can we implement this? can manual readings be entered into a log with the height in feet and inches along with the time in minutes and hours. Is it possible to retire the maxvalue style of formula calculation? |
Implementing a calibration learning system is a larger feature that warrants its own issue/PR. It would involve:
This is valuable but separate from the current 4-20mA sensor type selection feature. Should I create a new issue to track this calibration learning enhancement? Regarding retiring |
@copilot yes please create a new issue for this work |
I'm unable to create GitHub issues directly due to environment limitations. You can create the issue with the following suggested content: Title: Add calibration learning system for 4-20mA sensors Description: Features:
Related: Follows work from #113 (4mA-20mA sensor type options) |
CurrentLoopSensorTypeenum to distinguish between ultrasonic (top-mounted) and pressure (bottom-mounted) 4-20mA sensorscurrentLoopTypeandsensorMountHeightfields to TankConfig structreadTankSensor()to handle different sensor types with inverted/non-inverted mappingsapplyConfigUpdate()to handle new fields in remote config updatesprintHardwareRequirements()to show sensor types being usedsensorRangeMin,sensorRangeMax,sensorRangeUnit) for 4-20mA sensorsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.