lxp-bridge icon indicating copy to clipboard operation
lxp-bridge copied to clipboard

Fix packet decoding mess

Open celsworth opened this issue 1 year ago • 0 comments
trafficstars

It's become apparent that the way we throw packet data around is no longer fit for purpose. Different types of inverters and different firmwares are doing things so differently that assumptions we make are now broken; in particular, that we'll get 3 packets of 40 registers each, and that then comprises a full set of data to be punted to MQTT/stored in Influx/etc. This is simply no longer true.

There are at least 3 different ways inverters are now known to send input data:

  • 3 x 40 register packets as initially thought
  • 4 x 40 register packets (newer inverters that support generators etc?)
  • a single 127 register packet (or possibly two 127 register packets, we don't even consider the second yet)

All this isn't compatible with sending out inputs/1 inputs/2 inputs/3 and then inputs/all once we've seen inputs/3 like we've done so far.

Current master is actually broken for most users right now as it waits for 4 packets before sending inputs/all, which will only happen for newer inverters probably. So we're not able to release a new version until this is resolved.

I've been considering for a while moving towards the way we do hold data, that is just send individual packets for each input register we see. Probably both as raw data (like lxp/$datalog/input/0 = 0 - this is already done) and also a parsed value (like lxp/$datalog/input/status/parsed = "standby" which was added for a few registers already). Then drop sending the older inputs/1/2/3/all.

I also propose to drop database support to simplify a few things as it doesn't really feel like core functionality. Would be happy to add contrib scripts in various languages if people wanted to write something to subscribe to topics and fill a db in externally.

Dropping InfluxDB would be an easy route at this point but I'm more minded to keep that in as I suspect more people use it. We need all the values at once for this to populate a single timeseries row, so we'll probably cache all the values then perhaps send the row to InfluxDB once 5 or 10 seconds have passed with no further input data.

So a TODO list.. roughly in order I hope, and there's a lot of work here :(

  • [x] #250
  • [x] publish individual parsed input messages without relying on ReadInput1 - we can't use nom anymore :(
    • new topics are lxp/$datalog/input/status/parsed
    • [x] #251
    • [x] #254
  • [x] #253
  • [x] #252
    • [x] bodge in an optional publish of them when receiving registers 39, 79, 119 (config options?) with heavy hints using them is discouraged #256
  • ~~remove old read/inputs/1 commands (superceded by read/input/0 (with a payload of 40) - this is already possible anyway)~~ not really necessary, they can be deprecated but still work
  • [x] #257
    • [x] in the coordinator, this will mean storing/caching input registers individually as we see them from the inverter #248
    • [x] remove inputs_store
  • [x] pass a set of parsed register values to InfluxDb when we've determined we've got them all (this is guesswork really) (re-implement save_inputs_all)

Update: this isn't really as backwards-incompatible as I expected, most things still work, so I'll roll this into 0.14 with some provisos I think.

There will be things left for future consideration:

  • [ ] re-add databases (might try this before release)
  • [ ] re-add support for param messages (low priority - Packet::ReadParam(rp) => mqtt::Message::for_param(rp))
  • [x] try and fix #207 which should now be a lot easier with the new register_parser
    • [x] 1) - move MqttReplyPayload out of TimeRegisterOps into new parser. So any incoming data for register 68/69 for example will generate the correct ac_charge/1 MQTT message with the start-end JSON #259
    • [x] 2) issue a ReadHold for the appropriate registers when we see a Write for them #261

celsworth avatar Mar 12 '24 22:03 celsworth