Skip to content

drivers/mge-hid.c: refactor, treat ABM unknown as disabled#4

Merged
masterwishx merged 6 commits intomasterwishx:work/246-add-battery.charger.status+modefrom
desertwitch:work/246-add-battery.charger.status+mode
Nov 12, 2024
Merged

drivers/mge-hid.c: refactor, treat ABM unknown as disabled#4
masterwishx merged 6 commits intomasterwishx:work/246-add-battery.charger.status+modefrom
desertwitch:work/246-add-battery.charger.status+mode

Conversation

@desertwitch
Copy link
Copy Markdown

@desertwitch desertwitch commented Oct 27, 2024

I tried to illustrate here my code review/comments on the PR: networkupstools#2660

  1. We've seen non-ABM capable devices with partial ABM paths, these were previously sent into ABM parsing functions. Treating ABM_UNKNOWN as ABM_DISABLED resolves this problem, ABM parsing functions should only be executed if ABM is explicitly ABM_ENABLED. This also allows for graceful transition from ABM_UNKNOWN/ABM_DISABLED to ABM_ENABLED.

  2. Removed ABM_ENABLED_TYPE as an additional way of enabling ABM, this is just a textual value for the battery charger type and ABM disabled/enabled is already handled through the correct HID path (x.ABMEnable) in another function. An ABM battery charger type can still be ABM disabled, so it makes no sense to assume enable ABM based on the charger type.

  3. Improved the two ABM decision paths (old + new) with clear structure, comments, improved function and variable names.

  4. Improved the checking of ABM enabled-ness by checking for known values, rather than assigning any values directly.

  5. Improved debug messages to be more clear and condensed, as they seemed to be partially overlapping in some areas.

  6. Improved multiple implicitly casted variables to explicit casting in a single place where it felt correct to do so.

Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
…-add-battery.charger.status+mode

Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com>
@masterwishx masterwishx merged commit eb6eeb7 into masterwishx:work/246-add-battery.charger.status+mode Nov 12, 2024
@desertwitch desertwitch deleted the work/246-add-battery.charger.status+mode branch November 12, 2024 12:56
@desertwitch desertwitch restored the work/246-add-battery.charger.status+mode branch November 12, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants