Skip to content
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

drivers/mge-hid.c: comment regarding ChargerType #2704

Merged
merged 1 commit into from
Dec 1, 2024

Conversation

desertwitch
Copy link
Contributor

@desertwitch desertwitch commented Dec 1, 2024

follow-up for #2660
as explained in https://github.com/networkupstools/nut/pull/2660/files#r1838070677

The current state suggests we're sure about the principle and values of UPS.BatterySystem.Charger.ChargerType and that ABM-enabledness deductions could (theoretically) also be made from this HID path in the future. I've amended that in this PR, so that the current state of investigations is reflected in a separate in-depth comment, as to avoid future development going down a possibly wrong path due to what were perhaps premature assumptions on our part. 😉

@jimklimov jimklimov added the Eaton label Dec 1, 2024
@jimklimov jimklimov added this to the 2.8.3 milestone Dec 1, 2024
@jimklimov jimklimov added USB ECO/ESS/HE/ABM modes Non-trivial power supply management modes (ECO, ESS, HE, ABM etc.) labels Dec 1, 2024
@jimklimov jimklimov merged commit 368e734 into networkupstools:master Dec 1, 2024
22 of 24 checks passed
@desertwitch desertwitch deleted the 2660-follow-up branch December 1, 2024 18:36
@masterwishx
Copy link
Contributor

We already confirmed that UPS.BatterySystem.Charger.ChargerType is only for the info and UPS.BatterySystem.Charger.ABMEnable to switch ABM (Enable in HID path point to it) or you still not sure ?

in your case switching to 0 maybe bug in firmware?

@desertwitch
Copy link
Contributor Author

We already confirmed that UPS.BatterySystem.Charger.ChargerType is only for the info and UPS.BatterySystem.Charger.ABMEnable to switch ABM (Enable in HID path point to it) or you still not sure ?

in your case switching to 0 maybe bug in firmware?

I removed the variable from the ABM table (in the comments) because it made no sense to be there when we're not using it for ABM enabled/disabled (and just as informational variable). The additional comments are just to document our investigation for the future (as it may be hard to find inside the very long PR discussion).

@masterwishx
Copy link
Contributor

The additional comments are just to document our investigation for the future (as it may be hard to find inside the very long PR discussion).

Cool , Sorry . By mistake from comment i was thinking you are not sure if it info variable :)

@masterwishx
Copy link
Contributor

I removed the variable from the ABM table (in the comments) because it made no sense to be there when we're not using it for ABM enabled/disabled (and just as informational variable).

Other values like :
UPS.BatterySystem.Charger.Mode etc also represent only info values !?
So UPS.BatterySystem.Charger.ChargerType represent info of charger type also some extensions .
so not sure why to remove it :(

@desertwitch
Copy link
Contributor Author

desertwitch commented Dec 1, 2024

I removed the variable from the ABM table (in the comments) because it made no sense to be there when we're not using it for ABM enabled/disabled (and just as informational variable).

Other values like :

UPS.BatterySystem.Charger.Mode etc also represent only info values !?

So UPS.BatterySystem.Charger.ChargerType represent info of charger type also some extensions .

so not sure why to remove it :(

The other variables all have a practical effect on either ABM enabled/disabled or the UPS status itself (charging/discharging). Putting it into this table with them, like it was, is misleading because one could then assume that it can also be used to determine ABM enabled/disabled, when in fact it is only a textual informational variable (for now). It is only for better logical separation in the comments, no code changes were made here. But maybe in 10 years someone will read this table having no idea about our previous discussions and make wrong deductions from it, that's what I wanted to avoid with this PR.

@masterwishx
Copy link
Contributor

But maybe in 10 years someone will read this table having no idea about our previous discussions and make wrong deductions from

OK then I hope your comment about will be enough, I just wanted not to loose from table this var that it represent type of charger...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Eaton ECO/ESS/HE/ABM modes Non-trivial power supply management modes (ECO, ESS, HE, ABM etc.) USB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants