refactor(battery_simulator): clean up param SIM_BAT_ENABLE#26823
refactor(battery_simulator): clean up param SIM_BAT_ENABLE#26823
Conversation
cdb320a to
1f4a575
Compare
|
No broken links found in changed files. |
disable instead with SIM_BAT_DRAIN <= 0
1f4a575 to
b73d5ff
Compare
|
Rebased on main (146f2b2) |
dakejahl
left a comment
There was a problem hiding this comment.
CI failures are unrelated
| default: 60 | ||
| min: 1 | ||
| min: 0 | ||
| max: 86400 |
There was a problem hiding this comment.
this isn't correct since the source code is constraining it to a max of 1
There was a problem hiding this comment.
The min of 0 is needed to turn it off when 0, so the code does not run in that case.
But the constraining in the first place is not needed anymore since we changed from disable on -1 to disable on 0, so I removed it. 6380c4f (we are safe from division by zero as param compare has a tolerance of 1e-7)
There was a problem hiding this comment.
I was referring to the max value, but I was confused about what this parameter does. We should indeed add the clamp in source so avoid a divide by zero.
There was a problem hiding this comment.
Ah okay, sorry for the persistent misunderstandings!
- I think we can get rid of the max entirely actually, no reason to limit it at an arbitrary value.
- Added back the constraining to a min of 1 (you're right there are still cases where it is beneficial: starting the module and then changing the param to 0...)
- Commited your rewordings, thanks!
now that SIM_BAT_DRAIN=0 means the module is not started we are safe from division by zero again (param compare has a tolerance of 1e-7)
|
No broken links found in changed files. |
src/modules/simulation/battery_simulator/battery_simulator_params.yaml
Outdated
Show resolved
Hide resolved
src/modules/simulation/battery_simulator/battery_simulator_params.yaml
Outdated
Show resolved
Hide resolved
| default: 60 | ||
| min: 1 | ||
| min: 0 | ||
| max: 86400 |
There was a problem hiding this comment.
I was referring to the max value, but I was confused about what this parameter does. We should indeed add the clamp in source so avoid a divide by zero.
to avoid division by zero. This reverts commit 6380c4f.
Co-authored-by: Jacob Dahl <37091262+dakejahl@users.noreply.github.com>
Co-authored-by: Jacob Dahl <37091262+dakejahl@users.noreply.github.com>
ac6dbed to
8e1285a
Compare
Solved Problem
The param
SIM_BAT_ENABLEprovides very niche functionality (no battery sim at all).Solution
Remove it. Instead, disable the battery sim completely by setting
SIM_BAT_DRAIN<= 0.Also, add protection against division by 0 - in case somebody does run the battery sim but at runtime sets
SIM_BAT_DRAIN=0.Docs, param description, and lower limit updated accordingly.
Changelog Entry