write-safe register list

[divenal]

https://github.com/hoggyhoggy/givenergy-modbus-async/blob/85675ba2a0c03a4a6dd0f7f81d1d3cd65db2716b/givenergy_modbus/pdu/write_registers.py#L20 https://github.com/hoggyhoggy/givenergy-modbus-async/blob/85675ba2a0c03a4a6dd0f7f81d1d3cd65db2716b/givenergy_modbus/client/commands.py#L23

It's not ideal that there are separate list of registers scattered about the tree. Would be good to align it with the register definitions in inverter.py (Presuambly to a first approximation, if it's named in the inverter list of registers, it's safe to write to ?

Might possibly special-case reboot - it's not very safe to write to, since arbitrary values can cause problems ?

[divenal]

I wonder... if givTCP is officially sanctioned, perhaps it would be nice to be able to actually use their register.py file unmodified.

[divenal]

My proposal is add a field to the register definition with valid=[min,max], which implies it is writable. Might add an alias writable=True which is equiavent to range[0,65535]. So register definitions would look like

"battery_charge_limit": Def(C.uint16, None, HR(111), valid=[0, 50]),

Could also add an explicit writable=False which is primarily documentation - it is the absense of a valid range that means it's not writable.

And then add Plant.lookup_register_and_validate_value(name, val) which looks up the name in the InverterRegisterGetter table, and checks whether the value is within range. If so, it returns the integer holding register number, otherwise throws a ValueError.

This means commands doesn't need its own map of register name to number. It could have a helper function along the lines of

  def set_holding_register(name, value):
     idx = Plant.lookup_register_and_validate_value(name, value)
    return WriteHoldingRegisterRequest(idx, value)

allowing things like

def set_battery_charge_limit(val: int) -> list[TransparentRequest]:
    """Set the battery charge power limit as percentage. 50% (2.6 kW) is the maximum for most inverters."""
    return set_holding_register("battery_charge_limit", val)

Would WriteHoldingRegisterRequest() still need to do its own writabilty check, or is a check at the commands.py sufficient ? If it does need a check, would need to generate a reverse lookup table derived from the master register list, but keyed on register number rather than name.

Would be better if it was Inverter.lookup() rather than Plant.lookup() but Inverter seems to be some weird auto-generated class..? That's something I still need to understand a bit better.

[divenal]

There is a complication that some values span multiple registers. I'm not entirely convinced that turning pairs of start and stop times into a single TimeSlot value is particularly useful. Eg

        "module": Def(C.uint32, (C.hex, 8), HR(1), HR(2)),
       "charge_slot_1": Def(C.timeslot, None, HR(94), HR(95)),

We can only write one real register at a time. The lookup_register() function could use 'charge_slot_1:start' as a a way to reference the first of the pair, and ':end' for the second.

I'm not really sure what the 'module' is, but if it's not writable, that's fine. If there are any writable 32-bit values, could use :0 and :1 as qualifiers for those. (So ':start' is really just an alias of ':0' and ':stop' for ':1'. Could also accept ':end' as an alias for ':stop'.)

[hoggyhoggy]

Yes that was one thing that was bugging me, we seem to be repetitive in defining registers in variations places, (LUT, Commands, Inverter....etc) so purely from a maintenance perspective I think the aim should be to have all that in as few places as possible. GE Firmware updates tend to come with additional registers so would like to make it a "1 page wonder" rather than a chore!

[divenal]

For writable wide registers, I wonder if the simplest thing to do is just have multiple entries - one for the individual register, plus the wide one for those that want it.

        "charge_slot_2_start": Def(C.uint16, None, HR(31)), valid=(0,2359))
        "charge_slot_2_end": Def(C.uint16, None, HR(31)), valid=(0,2359))
        "charge_slot_2": Def(C.timeslot, None, HR(31), HR(32)),

Only the single-register ones are marked writable, the composite ones are read-only. Because the values are now computed lazily, there's little downside to having multiple ways to view a register.

(Unfortunately, a simple valid range isn't quite enough to validate a time, since this would allow 1262 or something. Inverter might treat that as 1302, but can't be sure. Maybe allow something like valid = 'HHMM')

Another way of doing this is to only have the start and end slots in the table, and doing the timeslot ones a different way: at the bottom of the file, there's some commented-out code

    # @computed('p_pv')
    # def compute_p_pv(p_pv1: int, p_pv2: int, **kwargs) -> int:
    #     """Computes the discharge slot 2."""
    #     return p_pv1 + p_pv2

I think computed is a pydantic thing, but equivalent could be

    @property
    def charge_slot_2(self) -> Timeslot:
         return Timeslot(self.p_pv1, self.p_pv2)

Then this will have the same effect: access to inverter.charge_slot_2 will return a TimeSlot containing start and end - same interface, just a slightly different implementation detail.

But given that we may as well stick with the existing multi-reg mechanism for things like serial numbers and 32-bit inputs, I'd probably just opt for the duplicate-entry approach.

[divenal]

A first cut in https://github.com/divenal/givenergy-modbus-async/commit/8a095de92877c3a1743cba74309cdd98d8a74de0

Still work in progress - I've not updated all the commands, annotated all the registers, or added a way to write start/end timeslots.

(Some seem targetted specifically at gen-1, like the 'storage' modes and things.)

[divenal]

I hadn't noticed that there's already precedents for having multiple entries for registers:

        "device_type_code": Def(C.hex, None, HR(0)),
        "inverter_max_power": Def(C.hex, C.inverter_max_power, HR(0)),
        "model": Def(C.hex, Model, HR(0)),
        ...
        "dsp_firmware_version": Def(C.uint16, None, HR(19)),
        "arm_firmware_version": Def(C.uint16, None, HR(21)),
        "generation": Def(C.uint16, Generation, HR(21)),
        "firmware_version": Def(C.firmware_version, None, HR(19), HR(21)),

which are all different ways of reading the same raw data, but in different formats. I'm not sure I'm at all convinced by this. Particularly the inverter_max_power which is computed from the model identifier. I think that really ought to be an explicit function where you get the model and then you call to find out the power. Similarly 'generation' which, I guess, tries to guess the hardware generation from the firmware number.

I definitely wouldn't consider these to be "interpretations" of the register contents. But I guess there's no end to the sort of shenanigans you can hide behind this conversion stuff.