Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements ERG mode tuning and improvements developed in collaboration with Marc Roy. The changes focus on refining PID control, improving power table lookup logic, and adding proper type conversions for sensor data.
Key Changes:
- Refactored ERG mode calculation to return position values instead of directly updating, enabling better control flow
- Added proper rounding for float-to-int conversions in sensor data (cadence and power)
- Improved ERG mode calculations with new PID gain adjustments, better time-to-target calculations, and negative result validation for homed systems
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SensorCollector.cpp | Added proper rounding when converting float sensor values to int; removed intermediate variables for speed and resistance |
| src/PowerTable_Helpers.cpp | Minor formatting: added blank line and removed redundant comment |
| src/ERG_Mode.cpp | Major refactoring: changed function signatures to return int32_t, added PID gain modulation based on error magnitude, improved time calculation for stepper movement, added sanity check for negative table results when homed |
| include/ERG_Mode.h | Updated function signatures for _setPointChangeState and _inSetpointState to return int32_t |
| CHANGELOG.md | Documented changes including PID tuning work, rounding improvements, and ERG tweaks |
| int adjustedTarget = newWatts.getTarget() >= newWatts.getValue() ? newWatts.getTarget() - POWERTABLE_WATT_INCREMENT : newWatts.getTarget() + POWERTABLE_WATT_INCREMENT; | ||
| int32_t tableResult = powerTable->lookup(adjustedTarget, newCadence); | ||
|
|
||
| // A lot of times this likes to undershoot going from High to low. Lets fix it. |
There was a problem hiding this comment.
The adjustedTarget is modified after the power table lookup has already occurred (line 161). The modification on line 165 won't affect the table lookup, but it will affect the subsequent comparisons on lines 170 and 174, as well as the log message on line 191. If this adjustment is intended to influence the lookup, it should be done before line 161. Otherwise, consider using a different variable name for the adjusted value to avoid confusion.
| int adjustedTarget = newWatts.getTarget() >= newWatts.getValue() ? newWatts.getTarget() - POWERTABLE_WATT_INCREMENT : newWatts.getTarget() + POWERTABLE_WATT_INCREMENT; | |
| int32_t tableResult = powerTable->lookup(adjustedTarget, newCadence); | |
| // A lot of times this likes to undershoot going from High to low. Lets fix it. | |
| int lookupTarget = newWatts.getTarget() >= newWatts.getValue() ? newWatts.getTarget() - POWERTABLE_WATT_INCREMENT : newWatts.getTarget() + POWERTABLE_WATT_INCREMENT; | |
| int32_t tableResult = powerTable->lookup(lookupTarget, newCadence); | |
| // A lot of times this likes to undershoot going from High to low. Lets fix it for the comparison target. | |
| int adjustedTarget = lookupTarget; |
| } | ||
|
|
||
| // Sanity check - with homing enabled, we should never have a negative result. If we do, something went wrong. | ||
| if (rtConfig->getHomed() && tableResult < 0) { |
There was a problem hiding this comment.
The condition "tableResult < 0" will be true when tableResult equals RETURN_ERROR (which is INT32_MIN, a negative value). This results in a redundant check and assignment when tableResult is already RETURN_ERROR. Consider adding "&& tableResult != RETURN_ERROR" to the condition on line 181 to avoid this redundant operation.
| if (rtConfig->getHomed() && tableResult < 0) { | |
| if (rtConfig->getHomed() && tableResult < 0 && tableResult != RETURN_ERROR) { |
| - Fixed bug where scans may not happen even when configured devices aren't connected. | ||
| - Worked with Mark Roy to tune PID. | ||
| - Added proper rounding from float to int for power and cadence. | ||
| - More ERG tweaks for Marc Roy. |
There was a problem hiding this comment.
Inconsistent spelling of the name "Marc Roy" vs "Mark Roy" in the changelog. Line 26 says "Mark Roy" while line 28 says "Marc Roy". These should be consistent.
| - More ERG tweaks for Marc Roy. | |
| - More ERG tweaks for Mark Roy. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks to @madooroy for his help and insight during testing. |
Co-Authored-By: madooroy <231097000+madooroy@users.noreply.github.com>
No description provided.