-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
RCT: improve battery control #26358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
RCT: improve battery control #26358
+1
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- Consider explaining or replacing the magic values
0,1032, and2048with named constants to make the valid battery operating modes self-documenting and easier to maintain. - Now that
batStatusis queried unconditionally, double-check that the extra request forapi.BatteryNormalmode is intentional and acceptable from a performance/latency perspective. - The new TRACE log for every
batteryModecall may be quite chatty; you might want to log only on state changes or unexpected statuses to keep trace output manageable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider explaining or replacing the magic values `0`, `1032`, and `2048` with named constants to make the valid battery operating modes self-documenting and easier to maintain.
- Now that `batStatus` is queried unconditionally, double-check that the extra request for `api.BatteryNormal` mode is intentional and acceptable from a performance/latency perspective.
- The new TRACE log for every `batteryMode` call may be quite chatty; you might want to log only on state changes or unexpected statuses to keep trace output manageable.
## Individual Comments
### Comment 1
<location> `meter/rct.go:143` </location>
<code_context>
+ if mode != api.BatteryNormal {
// check for normal operating mode
- if batStatus != 0 && batStatus != 1032 {
+ if batStatus != 0 && batStatus != 1032 && batStatus != 2048 {
return fmt.Errorf("invalid battery operating mode: %d", batStatus)
}
</code_context>
<issue_to_address>
**suggestion:** Replace magic numeric battery status values with named constants or enums for clarity and maintainability.
These literal values (`0`, `1032`, `2048`) are non-obvious and make the condition harder to read and maintain. If the rct package (or a related module) doesn’t provide symbolic values for these statuses, consider introducing named constants (e.g. `BatteryStatusNormal`, `BatteryStatusX`, `BatteryStatusY`) so the intent is clear and the values can be safely reused or updated in one place.
Suggested implementation:
```golang
package meter
const (
batteryStatusNormal = 0
batteryStatusCharging = 1032
batteryStatusDischarging = 2048
)
```
```golang
// check for normal operating mode
if batStatus != batteryStatusNormal &&
batStatus != batteryStatusCharging &&
batStatus != batteryStatusDischarging {
return fmt.Errorf("invalid battery operating mode: %d", batStatus)
```
1. Adjust the constant names (`batteryStatusNormal`, `batteryStatusCharging`, `batteryStatusDischarging`) if you have domain-specific terminology for these status codes (e.g. “float”, “bulk”, etc.), or if there are existing naming conventions/constants in this package.
2. If these status values are used elsewhere in the codebase, update those usages to reference the new constants for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
andig
reviewed
Jan 2, 2026
This reverts commit b55d30f.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref #25243 (reply in thread)
\cc @andig