ECOmode-auto-addon2-todo

[masterwishx]

Signed-off-by: DaRK AnGeL [email protected]

To continue work for #2949 :

To make automatic ECO Mode commands:

experimental.ecomode.start.auto
experimental.ecomode.stop.auto

that run Bypass On then ECO Mode Enable and back instead of users are run two separate commands or variables :

bypass.start + experimental.ecomode.enable
bypass.stop + experimental.ecomode.disable

or

 Enable bypass mode:
upsrw -u admin -p pass -s input.bypass.switch.on=on eaton

Enable ECO mode:
upsrw -u admin -p pass -s input.eco.switchable=enable eaton

Disable bypass mode:
upsrw -u admin -p pass -s input.bypass.switch.off=off eaton

Disable ECO mode:
upsrw -u admin -p pass -s input.eco.switchable=normal eaton

[masterwishx]

@jimklimov i made the new pr for aecomode auto ,we need to confirm what best name for commands :
if you think auto not good , maybe :

experimental.bypass.ecomode.start
experimental.bypass.ecomode.stop

[jimklimov]

Yes, if they are one-time commands that take effect when issued (and not a toggle for UPS to auto-change modes later based on environmental circumstances and this choice of strategy, like e.g. trim/boost happens automatically to compensate for input voltage outside low/high thresholds), then *.start and *.stop variants are better.

[masterwishx]

@ZivertX can you please test this pr : https://github.com/masterwishx/nut/tree/ECOmode-auto-addon2-todo when you will have time ?

This will run bypass + ecomode on/off by one command:

experimental.bypass.ecomode.start
experimental.bypass.ecomode.stop 

Please check (by ups screen or so) that bypass was enabled before ECO mode when on , also when off, bypass was disabled before exit from ecomode.

Also Please include debug Logs.
Thanks

[ZivertX]

experimental.bypass.ecomode.start
experimental.bypass.ecomode.stop 

Built your version (ECOmode-auto-addon2-todo)

driver.version: 2.8.2.2824.348-3172+g327e51e24

1. UPS doesn't go into bypass mode, but switches to ECO

experimental.bypass.ecomode.start.log

3. UPS doesn't do anything, ECO mode still active

experimental.bypass.ecomode.stop.log

[masterwishx]

1. UPS doesn't go into bypass mode, but switches to ECO

Thanks will try to fix it

[masterwishx]

driver.version: 2.8.2.

@jimklimov should it be 2.8.3 ?

[masterwishx]

driver.version: 2.8.2

why not in 2.8.3 ?

[masterwishx]

May 19 16:42:46 nut usbhid-ups[929]: [D2] entering main_instcmd(experimental.bypass.ecomode.start, (null)) for [eaton] on socket 10
May 19 16:42:46 nut usbhid-ups[929]: [D2] shared main_instcmd() does not handle command experimental.bypass.ecomode.start, proceeding to driver-specific handler
May 19 16:42:46 nut usbhid-ups[929]: [D1] instcmd(experimental.bypass.ecomode.start, [NULL])
May 19 16:42:46 nut usbhid-ups[929]: [D3] instcmd: using Path 'UPS.PowerConverter.Input.[5].Switchable'
May 19 16:42:46 nut usbhid-ups[929]: [D3] hu_find_valinfo: no matching HID value for this INFO_* value (1)

@jimklimov

nut/drivers/mge-hid.c

Lines 2167 to 2170 in 06d1032

	{ "experimental.bypass.ecomode.start", 0, 0, "UPS.PowerConverter.Input.[5].Switchable", NULL, "1", HU_TYPE_CMD, eaton_input_eco_mode_auto_on_off_info },
	/* Command to switch from ECO(HE) Mode with switch from Automatic Bypass Mode on before */
	{ "experimental.bypass.ecomode.stop", 0, 0, "UPS.PowerConverter.Input.[5].Switchable", NULL, "0", HU_TYPE_CMD, eaton_input_eco_mode_auto_on_off_info },

We still have error when sending function in last setting of cmd instead of NULL?

can HU_TYPE_CMD_PARAM_REQUIRED instead of HU_TYPE_CMD can help here ?

[jimklimov]

Regarding "2.8.2" - these strings from git describe depend on known git tags (probably had no git pull --tags after 2.8.3 release).

can HU_TYPE_CMD_PARAM_REQUIRED instead of HU_TYPE_CMD help here ?

No, it should require that you pass an argument (e.g. as upscmd usp@host cmd arg IIRC) if there is no (null) default in the table.

Here it finds "1" but then for some reason fails to use it. Can't say more ATM, am on phone in a bus...

[masterwishx]

Here it finds "1" but then for some reason fails to use it. Can't say more ATM, am on phone in a bus...

I will try to dig it out, but if you will have time to help here will be cool, not shure how it should work as we don't have any command with function in last parametr instead of Null. :(

[jimklimov]

Digging a bit :)

For reference, definitions of mapping table elements (as of latest commit in this PR branch) are:

• info_lkp_t (for info lookup helper methods):

  nut/drivers/usbhid-ups.h

  Lines 79 to 84 in 06d1032

  	typedef struct {
  	const long hid_value; /* HID value */
  	const char *nut_value; /* NUT value */
  	const char *(*fun)(double hid_value); /* optional HID to NUT mapping */
  	double (*nuf)(const char *nut_value); /* optional NUT to HID mapping */
  	} info_lkp_t;

• hid_info_t (for the main mapping table):

  nut/drivers/usbhid-ups.h

  Lines 176 to 192 in 06d1032

  	typedef struct {
  	const char *info_type; /* NUT variable name */
  	int info_flags; /* NUT flags (to set in addinfo) */
  	int info_len; /* if ST_FLAG_STRING: length of the string */
  	/* if HU_TYPE_CMD: command value */
  	const char *hidpath; /* Full HID Object path (or NULL for server side vars) */
  	HIDData_t *hiddata; /* Full HID Object data (for caching purpose, filled at runtime) */
  	const char *dfl; /* if HU_FLAG_ABSENT: default value ; format otherwise */
  	unsigned long hidflags; /* driver's own flags */
  	info_lkp_t *hid2info; /* lookup table between HID and NUT values */
  	/* if HU_FLAG_ENUM is set, hid2info is also used
  	* as enumerated values (dstate_addenum()) */
  	/* char *info_HID_format; *//* FFE: HID format for complex values */
  	/* interpreter interpret; *//* FFE: interpreter fct, NULL if not needed */
  	/* void *next; *//* next hid_info_t */
  	} hid_info_t;

For INSTCMD we are interested in such hid_info_t entries that include HU_TYPE_CMD in their hidflags.

They may have a default dfl command argument (if not NULL); in our two entries they do, "0" and "1" as strings that are later converted to numbers:

nut/drivers/mge-hid.c

Lines 2166 to 2169 in 06d1032

	/* Command to switch ECO(HE) Mode with switch to Automatic Bypass Mode on before */
	{ "experimental.bypass.ecomode.start", 0, 0, "UPS.PowerConverter.Input.[5].Switchable", NULL, "1", HU_TYPE_CMD, eaton_input_eco_mode_auto_on_off_info },
	/* Command to switch from ECO(HE) Mode with switch from Automatic Bypass Mode on before */
	{ "experimental.bypass.ecomode.stop", 0, 0, "UPS.PowerConverter.Input.[5].Switchable", NULL, "0", HU_TYPE_CMD, eaton_input_eco_mode_auto_on_off_info },

They may also refer to a further table info_lkp_t *hid2info (if not NULL).

The lookup fault in your log:

May 19 16:42:46 nut usbhid-ups[929]: [D2] entering main_instcmd(experimental.bypass.ecomode.start, (null)) for [eaton] on socket 10
May 19 16:42:46 nut usbhid-ups[929]: [D2] shared main_instcmd() does not handle command experimental.bypass.ecomode.start, proceeding to driver-specific handler
May 19 16:42:46 nut usbhid-ups[929]: [D1] instcmd(experimental.bypass.ecomode.start, [NULL])
May 19 16:42:46 nut usbhid-ups[929]: [D3] instcmd: using Path 'UPS.PowerConverter.Input.[5].Switchable'
May 19 16:42:46 nut usbhid-ups[929]: [D3] hu_find_valinfo: no matching HID value for this INFO_* value (1)

...seems to come from code path at

nut/drivers/usbhid-ups.c

Lines 914 to 931 in 06d1032

	/* Check if the item is an instant command */
	if (!(hidups_item->hidflags & HU_TYPE_CMD)) {
	upsdebugx(2, "instcmd: %s is not an instant command", cmdname);
	return STAT_INSTCMD_INVALID;
	}
	/* If extradata is empty, use the default value from the HID-to-NUT table */
	val = extradata ? extradata : hidups_item->dfl;
	if (!val && hidups_item->hidflags & HU_FLAG_PARAM_REQUIRED) {
	upsdebugx(2, "instcmd: %s requires an explicit or default parameter", cmdname);
	return STAT_INSTCMD_CONVERSION_FAILED;
	}
	/* Lookup the new value if needed */
	if (hidups_item->hid2info != NULL) {
	/* item->nuf() is expected to handle NULL if it must */
	value = hu_find_valinfo(hidups_item->hid2info, val);
	} else {

...and specifically it fails in hu_find_valinfo() at the last quoted line. That method looks into an info_lkp_t *hid2info (treated as a single element if (hid2info->fun != NULL), or a lookup array while info_lkp->nut_value != NULL otherwise... for apparently static lookups for) as seen at

nut/drivers/usbhid-ups.c

Lines 2636 to 2663 in 06d1032

	/* find the HID Item value matching that NUT value */
	/* useful for set with value lookup... */
	static long hu_find_valinfo(info_lkp_t *hid2info, const char* value)
	{
	info_lkp_t *info_lkp;
	/* if a conversion function is defined, use 'value' as argument for it */
	if (hid2info->nuf != NULL) {
	double hid_value;
	hid_value = hid2info->nuf(value);
	upsdebugx(5, "hu_find_valinfo: found %g (value: %s)", hid_value, value);
	return hid_value;
	}
	for (info_lkp = hid2info; info_lkp->nut_value != NULL; info_lkp++) {
	if (!(strcmp(info_lkp->nut_value, value))) {
	upsdebugx(5,
	"hu_find_valinfo: found %s (value: %ld)",
	info_lkp->nut_value, info_lkp->hid_value);
	return info_lkp->hid_value;
	}
	}
	upsdebugx(3,
	"hu_find_valinfo: no matching HID value for this INFO_* value (%s)",
	value);
	return -1;
	}

So apparently all other command cases which used some mapping function worked with a trivial mapping table of one or two info_lkp_t entries, one with a non-trivial nuf (which takes the given char* argument which may be NULL generally, and returns a double number... then converted to a long :\ ), and maybe a NULL,NULL,... sentinel as the other entry just in case - making a separate table for each unique function. And if there was no nuf in that (or no fun in the inverse hu_find_infoval()), the several couples of number(hid_value)+string(nut_value) had roles to play instead.

• fun(): double arg => char* return
• nuf(): char* arg => double(=>long) return

In your case, the eaton_input_eco_mode_auto_on_off_info[] mapping at

nut/drivers/mge-hid.c

Lines 1216 to 1222 in 06d1032

	/* High Efficiency (aka ECO) mode for auto start/stop commands */
	static info_lkp_t eaton_input_eco_mode_auto_on_off_info[] = {
	{ 0, "normal", eaton_input_eco_mode_auto_off_fun, NULL },
	{ 1, "ECO", eaton_input_eco_mode_auto_on_fun, NULL },
	{ 2, "ESS", NULL, NULL },
	{ 0, NULL, NULL, NULL }
	};

...defines two fun entries (apparently only the first would get used) but not nuf.

With current code structure, which apparently ignores fun/nuf in non-first entries, this requires that you split your function-oriented tables into several single-entry mappings (maybe with a sentinel to avoid overflows just in case), possibly defining both one fun and one nuf in that first entry for the opposite use-cases, and keep function-less mappings for the fixed string<->number lookups.

Thanks for making me dig into this - now I think this problem seems to impact your other tables with fun entries not at the first spot, e.g.

• eaton_input_eco_mode_on_off_info[] at

  nut/drivers/mge-hid.c

  Lines 1016 to 1022 in 06d1032

  	/* High Efficiency (aka ECO) mode, Energy Saver System (aka ESS) mode makes sense for UPS like (93PM G2, 9395P) */
  	static info_lkp_t eaton_input_eco_mode_on_off_info[] = {
  	{ 0, "normal", NULL, NULL },
  	{ 1, "ECO", eaton_input_eco_mode_check_range, NULL }, /* NOTE: "ECO" = tested on 9SX model and working fine, 9E model can stuck in ECO mode https://github.com/networkupstools/nut/issues/2719 */
  	{ 2, "ESS", eaton_input_ess_mode_report, NULL },
  	{ 0, NULL, NULL, NULL }
  	};

• info_lkp_t eaton_input_bypass_mode_on_info[] at

  nut/drivers/mge-hid.c

  Lines 1140 to 1145 in 06d1032

  	/* Automatic Bypass mode on */
  	static info_lkp_t eaton_input_bypass_mode_on_info[] = {
  	{ 0, "disabled", NULL, NULL },
  	{ 1, "on", eaton_input_bypass_check_range, NULL },
  	{ 0, NULL, NULL, NULL }
  	};

...and probably older mappings too, or did I misunderstand something when reading the methods above? CC @arnaudquette-eaton - please help us out here (and help fix Eaton support as relevant) :)
They are moderately impacted though, as every line refers to same fun/nuf methods so hitting them via first line when acting for a second one does not have practical impact; skipping the explicit string<->number conversion (or doing it different in table and in actually called methods) might be more confusing. For example:

• pegasus_yes_no_info[] at

  nut/drivers/mge-hid.c

  Lines 873 to 877 in 06d1032

  	static info_lkp_t pegasus_yes_no_info[] = {
  	{ 0, "no", pegasus_yes_no_info_fun, pegasus_yes_no_info_nuf },
  	{ 1, "yes", pegasus_yes_no_info_fun, pegasus_yes_no_info_nuf },
  	{ 0, NULL, NULL, NULL }
  	};

• pegasus_threshold_info[] at

  nut/drivers/mge-hid.c

  Lines 823 to 828 in 06d1032

  	static info_lkp_t pegasus_threshold_info[] = {
  	{ 10, "10", eaton_check_pegasus_fun, NULL },
  	{ 25, "25", eaton_check_pegasus_fun, NULL },
  	{ 60, "60", eaton_check_pegasus_fun, NULL },
  	{ 0, NULL, NULL, NULL }
  	};

Some summary thoughts:

• I did not check further, just enough to confirm there may be decades worth of confusion here
  • worth either a revision of existing mappings to use either single entries for each fun/nuf involved, or function-less static conversions,
  • or a careful redesign/rewrite of hu_find_valinfo()/hu_find_infoval() to consider the non-first entry methods - but this probably makes little sense, as a single facade method should be able to handle all argument values (and may dispatch to further methods for particular practical features where applicable).
  • MAAAYBE it makes sense to support raising e.g. an errno (or a driver-defined equivalent) when handling a fun()/nuf() when the method refuses to handle an argument value, so we would proceed with fixed table lookup?
  • It would anyhow be useful to know that the function disliked an argument, so we can return STAT_INSTCMD_FAILED, STAT_SET_INVALID, etc.
• Similar concerns may impact snmp-ups and/or nutdrv_qx with their take on such mapping tables.
  • Makes sense to revise what they do as well, and end up with some consistent approach across the board.
• Someone gotta take a look if any of this is clarified in the docs (they should be the first place to start... but might differ from actual code - something to fix in that case).
• The short-term fix for this PR is to revise the ECO mapping tables, so if there are functions to call - they are in the first entry of a table, and so there are separate tables for separate functions or a common dispatcher function calls further methods based on the argument ("normal", "ECO", "ESS" for nuf() here) - and somehow visibly rejects or quietly ignores any unsupported argument values.

Finally note that these functions seem designed to convert the arguments from one type to another, not so much to perform a series of active operations on the device (as we want in this PR, to chain several changes of HW/FW state in practice). There may be several correct-ish solutions to wedge this desire into existing framework:

• In instcmd(), after we get some value = hu_find_valinfo(hidups_item->hid2info, val); (or convert from the default number directly if there is no hid2info sub-mapping), we go on to HIDSetDataValue(udev, hidups_item->hiddata, value). So if the methods do something complex inside, keep in mind they should return a value that is reasonable, benign, and does not contradict the other activity.
• Limit the mapping methods to actual data conversions and no interaction with the device, and for complex operations like this - dstate_addcmd("experimental.bypass.ecomode.start") perhaps during mge_claim() AND ensure the subdriver has a say in instcmd() processing from the main driver - but I see no precedent for such approach in neither drivers/*-hid.c, nor drivers/*-mib.c, nor drivers/nutdrv_qx_*.c -- so probably now is not a good time to start with that. Mapping tables are the vehicle we have somehow functional and well-tested here.

[masterwishx]

For reference, definitions of mapping table elements (as of latest commit in this PR branch) are:

Yep i checked this but seems i forgot a little how its working :)

...and specifically it fails in hu_find_valinfo() at the last quoted line. That method looks into an info_lkp_t *hid2info (treated as a single element if (hid2info->fun != NULL), or a lookup array while info_lkp->nut_value != NULL otherwise... for apparently static lookups for) as seen at

Thanks will look into it

Thanks for making me dig into this - now I think this problem seems to impact your other tables with fun entries not at the first spot, e.g.

Strange those functions working fine from the tests that we had with @ZivertX confirmed by for bypass and ecomode ( for both upsrw and cmd (with NULL)) :

nut/drivers/mge-hid.c

Lines 1016 to 1022 in 06d1032

	/* High Efficiency (aka ECO) mode, Energy Saver System (aka ESS) mode makes sense for UPS like (93PM G2, 9395P) */
	static info_lkp_t eaton_input_eco_mode_on_off_info[] = {
	{ 0, "normal", NULL, NULL },
	{ 1, "ECO", eaton_input_eco_mode_check_range, NULL }, /* NOTE: "ECO" = tested on 9SX model and working fine, 9E model can stuck in ECO mode https://github.com/networkupstools/nut/issues/2719 */
	{ 2, "ESS", eaton_input_ess_mode_report, NULL },
	{ 0, NULL, NULL, NULL }
	};

nut/drivers/mge-hid.c

Lines 1140 to 1145 in 06d1032

	/* Automatic Bypass mode on */
	static info_lkp_t eaton_input_bypass_mode_on_info[] = {
	{ 0, "disabled", NULL, NULL },
	{ 1, "on", eaton_input_bypass_check_range, NULL },
	{ 0, NULL, NULL, NULL }
	};

But will check again with your info :)

Finally note that these functions seem designed to convert the arguments from one type to another, not so much to perform a series of active operations on the device (as we want in this PR, to chain several changes of HW/FW state in practice). There may be several correct-ish solutions to wedge this desire into existing framework:

Wow i need more time to check it :)

[jimklimov]

Disregard netbsd agent CI faults, the worker had an environmental issue.

[desertwitch]

Might be good to prefix all these upsdebugx with %s: and __func__ so we can see more easily which functions get called and do what during the testing and also later when in production.

[masterwishx]

upsdebugx(1, "%s: Bypass switch off state is: %s , must be disabled before switching off", __func__, bypass_switch_off_str); its ok ?

[desertwitch]

Yes that's how I usually do it so it shows which function emits the message.

[masterwishx]

Done.

[masterwishx]

Yes that's how I usually do it so it shows which function emits the message.

Yep i also did it but for some of upsdebugx in functions but forgot from back then about it and how stuff was working :(

[masterwishx]

Yes that's how I usually do it so it shows which function emits the message.

Maybe you can check about current issue we have here if you have time ?

[AppVeyorBot]

❌ Build nut 2.8.3.3195-master failed (commit b7d6731626 by @masterwishx)