EATON HID: add missing usages and paths (addon)

[masterwishx]

Adding HE/ECO mode, Bypass On/Off for Eaton USB UPS Online Models + Adding Missing usages and paths

Continue work/addon for #2637
Fixes #2485
Ref #2495

Added:

Variables: 

battery.charger.status: discharging
device.usb.version: 01.29
input.eco.switchable: normal
outlet.1.designator: AC OUTPUT
outlet.1.protect.status: protected
input.transfer.forced: enabled
input.eco.switchable: ECO
input.bypass.switch.on: off
etc ...

R/W variables:

input.eco.switchable
input.bypass.switch.on
etc ..

Commands: 

ecomode.disable - Take the UPS out of High Efficiency (aka ECO) mode
ecomode.enable - Put UPS in High Efficiency (aka ECO) mode
essmode.disable - Take the UPS out of Energy Saver System (aka ESS) mode
essmode.enable - Put UPS in Energy Saver System (aka ESS) mode
bypass.start - Put the UPS in bypass mode
bypass.stop - Take the UPS out of bypass mode

Added ABM statuses for 9E and other Models with next paths:

UPS.BatterySystem.Charger.ChargerType
UPS.BatterySystem.Charger.Status

[masterwishx]

The only difference seems treating ABM_UNKNOWN as ABM_DISABLED. If in your case x.ABMEnable does not exist, and you also don't have x.ChargerType=4 setting ABM_ENABLED_TYPE (which, to be honest, I'm not sure is correct as I don't see that in the HID table... but I kept your logic there), your UPS would be ABM_UNKNOWN and the ABM function would not run (correctly so).

I have x.ChargerType=4 also you have, but I don't have ABM x.ABMEnable.

And I have floating when battery.charge =100
After charge so this hid table working fine for me, that was the logic for fix 9E

[masterwishx]

If your UPS has a certain combination (e.g. "x.ChargerType=5" and "x.Status=2" without x.ABMEnable)

I have "x.ChargerType=4" it's constant charge (CC) if "x.Status=2" then it charging

[desertwitch]

The only difference seems treating ABM_UNKNOWN as ABM_DISABLED. If in your case x.ABMEnable does not exist, and you also don't have x.ChargerType=4 setting ABM_ENABLED_TYPE (which, to be honest, I'm not sure is correct as I don't see that in the HID table... but I kept your logic there), your UPS would be ABM_UNKNOWN and the ABM function would not run (correctly so).

I have x.ChargerType=4 also you have, but I don't have ABM x.ABMEnable.

And I have floating when battery.charge =100 After charge so this hid table working fine for me, that was the logic for fix 9E

If your UPS has a certain combination (e.g. "x.ChargerType=5" and "x.Status=2" without x.ABMEnable)

I have "x.ChargerType=4" it's constant charge (CC) if "x.Status=2" then it charging

OK, so that clears up where the ABM_ENABLED_TYPE originates from, so we know that to be enabled ABM.

But this logic is preserved the same in my code (see links):
https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L1623-L1629
https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L400-L416
https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L439-L449

when compared to your code:

nut/drivers/mge-hid.c

Lines 1627 to 1632 in ebc0ff4

	/* ABM (Advanced Battery Monitoring) processing
	* Must be processed before the BOOL status */
	/* Used to process ABM flag, for battery.charger.type */
	{ "battery.charger.type", 0, 0, "UPS.BatterySystem.Charger.ChargerType", NULL, "%.0f", HU_FLAG_QUICK_POLL, eaton_charger_type_info },
	/* Not published, just to store in internal var. advanced_battery_monitoring */
	{ "battery.charger.abm.status", 0, 0, "UPS.BatterySystem.Charger.ABMEnable", NULL, "%.0f", HU_FLAG_QUICK_POLL, eaton_abm_enabled_info },

nut/drivers/mge-hid.c

Lines 395 to 411 in ebc0ff4

	/* Used to process ABM flag, for battery.charger.type */
	static const char *eaton_abm_charger_type_fun(double value)
	{
	int abm_charger_type = value;
	if (abm_charger_type == ABM_ENABLED_TYPE)
	{
	/* Set ABM flag for battery.charger.type */
	advanced_battery_monitoring = ABM_ENABLED_TYPE;
	upsdebugx(2, "ABM charger type: %i", (int)value);
	return "ABM";
	}
	/* Handle the case when value is not equal to ABM_ENABLED_TYPE_MODE or some other appropriate action */
	return NULL;
	}

nut/drivers/mge-hid.c

Lines 431 to 450 in ebc0ff4

	if ((abm_charger_table_status == ABM_CHARGER_ENABLED) && (abm_charger_table_mode != ABM_CHARGER_ENABLED))
	{
	switch ((long)value)
	{
	case 1: /* charging status, FIXME: 9E Model - no chrg in ups.status when battery.charge < 100 */
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "chrg");
	break;
	case 2: /* floating status, FIXME: 9E Model - chrg not stop in ups.status after battery.charge = 100 */
	if (advanced_battery_monitoring == ABM_UNKNOWN)
	{
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "!chrg");
	break;
	}
	else
	{
	/* charging status, floating status */
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "chrg");
	break;
	}

Which leads me back to the question if you tested it with my code and it's not working? 😉
If so, please provide the debug logs and maybe we can find some common ground on what to improve.

[masterwishx]

Which leads me back to the question if you tested it with my code and it's not working?

Your code will not work for me if you make ABM_UNKNOWN with ABM_DISABLED to return.

I checked this befor when tryed same way, aslo original code was not working for me becose always (!chrg and ! dschrg)
I will post logs later...

[masterwishx]

OK, so that clears up where the ABM_ENABLED_TYPE originates from, so we know that to be enabled ABM.

This only to show table of ABM battery type

[masterwishx]

Also X.Status in you case =19, this wrong value we should use X.mode in your case.
And we can have ups that have only one of thouse.
So that was the logic in my code:

To not change original code but to add fix for 9E

[masterwishx]

this was in original code :

Path: UPS.PowerSummary.PresentStatus.Discharging, Type: Feature, ReportID: 0x01, Offset: 24, Size: 8, Value: 0
process_boolean_info: !dischrg

Path: UPS.PowerSummary.PresentStatus.Charging, Type: Feature, ReportID: 0x01, Offset: 16, Size: 8, Value: 1
 process_boolean_info: !chrg

this is my code that fine:

Path: UPS.BatterySystem.Charger.ChargerType, Type: Feature, ReportID: 0x26, Offset: 0, Size: 8, Value: 5
Set ABM variable to unknown, charger type status: -1
ABM is disabled

Path: UPS.BatterySystem.Charger.Status, Type: Feature, ReportID: 0x26, Offset: 8, Size: 8, Value: 2
ABM numeric status: 2
ABM string status: floating
ABM is -1

Path: UPS.PowerSummary.PresentStatus.Discharging, Type: Feature, ReportID: 0x01, Offset: 24, Size: 8, Value: 0
process_boolean_info: !dischrg

Path: UPS.PowerSummary.PresentStatus.Charging, Type: Feature, ReportID: 0x01, Offset: 16, Size: 8, Value: 1
process_boolean_info: !chrg

Path: UPS.BatterySystem.Charger.Status, Type: Feature, ReportID: 0x26, Offset: 8, Size: 8, Value: 2
ABM CHRG/DISCHRG legacy string status (ups.status): !chrg
process_boolean_info: !chrg

[masterwishx]

OK, so that clears up where the ABM_ENABLED_TYPE originates from, so we know that to be enabled ABM.

i wrote you before inyour code that here you break the code logic :
https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L434

[masterwishx]

OK, so that clears up where the ABM_ENABLED_TYPE originates from, so we know that to be enabled ABM.

i wrote you before inyour code that here you break the code logic : https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L434

Also here for status not floating :

https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L443

and all same ...

[desertwitch]

this is my code that fine:

Path: UPS.BatterySystem.Charger.ChargerType, Type: Feature, ReportID: 0x26, Offset: 0, Size: 8, Value: 5
Set ABM variable to unknown, charger type status: -1
ABM is disabled

Path: UPS.BatterySystem.Charger.Status, Type: Feature, ReportID: 0x26, Offset: 8, Size: 8, Value: 2
ABM numeric status: 2
ABM string status: floating
ABM is -1

Path: UPS.PowerSummary.PresentStatus.Discharging, Type: Feature, ReportID: 0x01, Offset: 24, Size: 8, Value: 0
process_boolean_info: !dischrg

Path: UPS.PowerSummary.PresentStatus.Charging, Type: Feature, ReportID: 0x01, Offset: 16, Size: 8, Value: 1
process_boolean_info: !chrg

Path: UPS.BatterySystem.Charger.Status, Type: Feature, ReportID: 0x26, Offset: 8, Size: 8, Value: 2
ABM CHRG/DISCHRG legacy string status (ups.status): !chrg
process_boolean_info: !chrg

This shows exactly that your UPS is ABM_UNKNOWN and yet continues to process ABM values as if it were ABM_ENABLED, which are then cleared out with your hard-coded if-clauses due to actually being in ABM_UNKNOWN state (which makes no sense to me):

nut/drivers/mge-hid.c

Lines 439 to 450 in ebc0ff4

	case 2: /* floating status, FIXME: 9E Model - chrg not stop in ups.status after battery.charge = 100 */
	if (advanced_battery_monitoring == ABM_UNKNOWN)
	{
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "!chrg");
	break;
	}
	else
	{
	/* charging status, floating status */
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "chrg");
	break;
	}

Floating should result in chrg and not !chrg at value 2 of UPS.BatterySystem.Charger.Status according to the HID table. I do not understand why you are forcing !chrg here for any devices ABM_UNKNOWN (or even your own device, for that matter).

Your code will not work for me if you make ABM_UNKNOWN with ABM_DISABLED to return.

i wrote you before inyour code that here you break the code logic : https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L434

Yes, and that is exactly my point, that - I think - it should not work (still run an ABM function) in such a case where your UPS (and any other UPS) is ABM_UNKNOWN. But rather we should find out the exact HID combination that your UPS uses for enabled ABM and make that ABM_ENABLED. My point is, just because still running the ABM function in an unknown ABM state works for your device, doesn't mean it won't break others and - I think - we should be careful here as this will affect many other devices also.

Also X.Status in you case =19, this wrong value we should use X.mode in your case.

Yes, and X.mode is used by both your code and my code (see my comment above).

Also here for status not floating :

https://github.com/masterwishx/nut/blob/e4f263171a3112024e270beda4961b66fad1c708/drivers/mge-hid.c#L443

and all same ...

Exactly the same as in your code:

nut/drivers/mge-hid.c

Lines 435 to 437 in ebc0ff4

	case 1: /* charging status, FIXME: 9E Model - no chrg in ups.status when battery.charge < 100 */
	snprintf(mge_scratch_buf, sizeof(mge_scratch_buf), "%s", "chrg");
	break;

Last, I'm still not convinced about the extra ABM-enabling code of ABM_ENABLED_TYPE, which equates UPS.BatterySystem.Charger.ChargerType with value 4 to an enabled ABM. Where is this coming from? The code comments suggest my 5P, but it actually controls it through UPS.BatterySystem.Charger.ABMEnable? Your device seems to use 5?

nut/drivers/mge-hid.c

Lines 395 to 421 in ebc0ff4

	/* Used to process ABM flag, for battery.charger.type */
	static const char *eaton_abm_charger_type_fun(double value)
	{
	int abm_charger_type = value;
	if (abm_charger_type == ABM_ENABLED_TYPE)
	{
	/* Set ABM flag for battery.charger.type */
	advanced_battery_monitoring = ABM_ENABLED_TYPE;
	upsdebugx(2, "ABM charger type: %i", (int)value);
	return "ABM";
	}
	/* Handle the case when value is not equal to ABM_ENABLED_TYPE_MODE or some other appropriate action */
	return NULL;
	}
	static info_lkp_t eaton_charger_type_info[] = {
	{ 0, "None", NULL, NULL },
	{ 1, "Extended (CLA)", NULL, NULL },
	{ 2, "Large extension", NULL, NULL },
	{ 3, "Extra large extension (XL)", NULL, NULL },
	{ 4, "ABM", eaton_abm_charger_type_fun, NULL },
	{ 5, "Constant Charge (CC)", NULL, NULL },
	{ 0, NULL, NULL, NULL }
	};

Anyway, I'm (respectfully) signing off for the day (it is Sunday after all). No offense, I'm really not trying to blast your code and I have great respect for the effort you've put into this, just trying to explain my reservations. It's also up to Jim eventually, I'm also just a contributor (same as you) here and it's just my personal opinion on where safeguards and improvements could be done.

For reference, my suggestions remain at: masterwishx#4

[masterwishx]

But rather we should find out the exact HID combination that your UPS uses for enabled ABM and make that ABM_ENABLED. My point is, just because still running the ABM function in an unknown ABM state works for your device, doesn't mean it won't break others and - I think - we should be careful here as this will affect many other devices also.

I think you still not get the point of my code :)
My 9E model don't have ABM but have type and status tables.
So I checked and my code work well now on 9E model so I think should work for other UPS that have same tables and ABM_UNKNOWN , for all other the code same as original.

So the point of code just to make fix for 9E and other with same tables and leave original code as it already working for any other upses

Offcourse @jimklimov will decide what's better looks.
I really thanks to you for your help here and offcourse for continuing and updating amazing plugin for Unraid

[masterwishx]

Floating should result in chrg and not !chrg at value 2 of UPS.BatterySystem.Charger.Status according to the HID table. I do not understand why you are forcing !chrg here for any devices ABM_UNKNOWN (or even your own device, for that matter).

Becouse otherwise I will have OL CHRG every time also after battery.charge = 100, but when battery.charge = 100 change from 99, I got floating instead of charging in status table.
Also OB DSCHRG when on battery

Or like was in original code it was always send (!chrg /!dschrg)

[masterwishx]

Last, I'm still not convinced about the extra ABM-enabling code of ABM_ENABLED_TYPE, which equates UPS.BatterySystem.Charger.ChargerType with value 4 to an enabled ABM. Where is this coming from? The code comments suggest my 5P, but it actually controls it through UPS.BatterySystem.Charger.ABMEnable? Your device seems to use 5?

This part not really important Becouse if 4 ABM just can only print debug and flag will be owerwriten by ABM enabled to 1 if ABM exist.

My device use 5 constant charge (CC)

Was needed to check if ABM enabled by other than ABM Enabled.
But seems to enable/disable ABM only ABM enabled used.
This was needed at start, and now I think if we don't use this flag, this function can be removed.

[desertwitch]

Last, I'm still not convinced about the extra ABM-enabling code of ABM_ENABLED_TYPE, which equates UPS.BatterySystem.Charger.ChargerType with value 4 to an enabled ABM. Where is this coming from? The code comments suggest my 5P, but it actually controls it through UPS.BatterySystem.Charger.ABMEnable? Your device seems to use 5?

This part not really important Becouse if 4 ABM just can only print debug and flag will be owerwriten by ABM enabled to 1 if ABM exist.

My device use 5 constant charge (CC)

Was needed to check if ABM enabled by other than ABM Enabled. But seems to enable/disable ABM only ABM enabled used. This was needed at start, and now I think if we don't use this flag, this function can be removed.

Ok, thanks for the clarification, so I think it's better if we remove this additional ABM-enabling code.
ABM seems to be enabled through UPS.BatterySystem.Charger.ABMEnable only and this should suffice? Just because the charger type is ABM(-capable), it doesn't have to be enabled either probably?

I think you still not get the point of my code :)
My 9E model don't have ABM but have type and status tables.

Your pull request description says: "Added ABM statuses for 9E", so I always assumed your UPS supports ABM and you are trying to make that work with the driver (which I factored into my code reviews and suggestions). But now I see that your UPS doesn't support ABM, which is also the reason you're getting 5 = Constant Charge (CC) for UPS.BatterySystem.Charger.ChargerType. So why are we modifying the existing ABM functions to "support" a device that does not have ABM? What values do you want to gain here and how can a/your UPS be "floating" if it's constant charge and does not have ABM?

I see the benefit in adding the additional ABM path for other ABM-capable devices, but if I'm not mistaken your device shouldn't use an ABM path/lookup table if it has no support for ABM...? So in that sense my improvement suggestion of not letting an ABM_UNKNOWN device (like yours) proceed into an ABM function was kind of correct...?

Path: UPS.PowerSummary.PresentStatus.Discharging, Type: Feature, ReportID: 0x01, Offset: 24, Size: 8, Value: 0
Path: UPS.PowerSummary.PresentStatus.Charging, Type: Feature, ReportID: 0x01, Offset: 16, Size: 8, Value: 1

Your device exposes the correct non-ABM paths according to your logs, and those correctly result in OL CHRG for your constant charge type charger, why the ABM changes?

[masterwishx]

Ok, thanks for the clarification, so I think it's better if we remove this additional ABM-enabling code.
ABM seems to be enabled through UPS.BatterySystem.Charger.ABMEnable only and this should suffice? Just because the charger type is ABM(-capable), it doesn't have to be enabled either probably?

Yes, but for other part it seems we are going around , maybe you will get it better if you will read from above comments of commits ?
#2660 (comment)

From this list you see ABM Eaton models , so mostly what left we have 5E = ABM_UNKNOWN and 9E = ABM_UNKNOWN + x.Status and x.Type tables .
We dont know what of these use x.Status or x.Mode

i will try again to explain : For statuses i added missing path for x.Type x.Status etc but i have leaved the original code as is only added fix for 9E Models ! by added :

if (x.status and not x.mode) 
  if unknown then change status of charge and discharge

So unknown only will have same logic as original code also other when abm_enabled + x.status or x.modes

[masterwishx]

I see the benefit in adding the additional ABM path for other ABM-capable devices, but if I'm not mistaken your device shouldn't use an ABM path/lookup table if it has no support for ABM...? So in that sense my improvement suggestion of not letting an ABM_UNKNOWN device (like yours) proceed into an ABM function was kind of correct...?

Yes you right but as i found the x.status table is working fine when charging/floating/discharging however no ABM in real so it let me fix the charging states aslo to see real charging status by this table !

Maybe its like firmware cuted from 9SX that have ABM , i dont know but its working , but not regular chrg/dischrg statuses.
Sorry i didnt find any other way to fix it ....
Also i didnt modified the original code that already works for other cases.

in any case again im sorry not started for this another pr than eco :(

[masterwishx]

correctly result in OL CHRG for your constant charge type charger

Always this state? Not shure it's correct, also have OB DSCHRG in this case.

[masterwishx]

now with my code by x.status have that looks more right than always OL CHRG:

[desertwitch]

No need to apologize, I appreciate your point of view and effort. To me it just doesn't seem to matter what ABM paths are exposed by devices that are not ABM-capable. I think it's not an improvement to parse ABM paths just because they are there (due to common firmware, leftovers, ...), when the device itself doesn't support ABM. A non-ABM device reporting as "floating" seems to me more of a bug than an improvement. And yes, I'd expect a constant charge type charger to always display either OL CHRG (constant charge) or OB DISCHRG and this lines up with what the HID table says for constant mode too. So the non-ABM function already displays correctly your non-ABM paths (before this PR), in my opinion, and adding ABM into the mix makes no sense for your device - I think.

[masterwishx]

No need to apologize, I appreciate your point of view and effort. To me it just doesn't seem to matter what ABM paths are exposed by devices that are not ABM-capable. I think it's not an improvement to parse ABM paths just because they are there (due to common firmware, leftovers, ...), when the device itself doesn't support ABM. A non-ABM device reporting as "floating" seems to me more of a bug than an improvement. And yes, I'd expect a constant charge type charger to always display either OL CHRG (constant charge) or OB DISCHRG and this lines up with what the HID table says for constant mode too. So the non-ABM function already displays correctly your non-ABM paths (before this PR), in my opinion, and adding ABM into the mix makes no sense for your device - I think.

The original code was always send !chrg and !dischrg so i was not see any charge and discharge at all

[desertwitch]

No need to apologize, I appreciate your point of view and effort. To me it just doesn't seem to matter what ABM paths are exposed by devices that are not ABM-capable. I think it's not an improvement to parse ABM paths just because they are there (due to common firmware, leftovers, ...), when the device itself doesn't support ABM. A non-ABM device reporting as "floating" seems to me more of a bug than an improvement. And yes, I'd expect a constant charge type charger to always display either OL CHRG (constant charge) or OB DISCHRG and this lines up with what the HID table says for constant mode too. So the non-ABM function already displays correctly your non-ABM paths (before this PR), in my opinion, and adding ABM into the mix makes no sense for your device - I think.

The original code was always send !chrg and !dischrg so i was not see any charge and discharge at all

You're right, my bad, because it incorrectly executed the ABM function when seeing your partial/unused ABM paths despite being ABM_UNKNOWN (even before your PR). This would be solved by treating ABM_UNKNOWN as ABM_DISABLED, as I suggested, and make it correctly use the non-ABM function for your device.

[masterwishx]

No need to apologize, I appreciate your point of view and effort. To me it just doesn't seem to matter what ABM paths are exposed by devices that are not ABM-capable. I think it's not an improvement to parse ABM paths just because they are there (due to common firmware, leftovers, ...), when the device itself doesn't support ABM. A non-ABM device reporting as "floating" seems to me more of a bug than an improvement. And yes, I'd expect a constant charge type charger to always display either OL CHRG (constant charge) or OB DISCHRG and this lines up with what the HID table says for constant mode too. So the non-ABM function already displays correctly your non-ABM paths (before this PR), in my opinion, and adding ABM into the mix makes no sense for your device - I think.

The original code was always send !chrg and !dischrg so i was not see any charge and discharge at all

You're right, my bad, because it incorrectly executed the ABM function when seeing your partial/unused ABM paths despite being ABM_UNKNOWN (even before your PR). This would be solved by treating ABM_UNKNOWN as ABM_DISABLED, as I suggested, and make it correctly use the non-ABM function for your device.

Yes but dont you think UPS Status Timeline in picture looks more right than always charge status ?
#2660 (comment)

[masterwishx]

Maybe @jimklimov and @arnaudquette-eaton can Reveal more about the right condition in this case?

[desertwitch]

Yes but dont you think UPS Status Timeline in picture looks more right than always charge status ?

#2660 (comment)

For a non-ABM device in constant charge mode it doesn't look correct to me, because my understanding of constant charge is that it's constantly applying charge (= always charging). This also aligns with the HID table and non-ABM paths exposed by your device, which also say it's charging (and are the only paths we should be looking at with a non-ABM-capable device imo):

Path: UPS.PowerSummary.PresentStatus.Discharging, Type: Feature, ReportID: 0x01, Offset: 24, Size: 8, Value: 0
Path: UPS.PowerSummary.PresentStatus.Charging, Type: Feature, ReportID: 0x01, Offset: 16, Size: 8, Value: 1

Let's see what the others say... 🙂

Meanwhile I've updated my improvement recommendations: masterwishx#4

[masterwishx]

Let's see what the others say.

If you have time meanwhile, maybe you can check what values for our paths your ups will send in X.type=5 (cc) mode When ABM is disabled also to check statuses?