manio / skymax-demo

https://skyboo.net/2017/03/monitoring-voltronic-power-axpert-mex-inverter-under-linux/
GNU General Public License v2.0
52 stars 29 forks source link

adding replylen parameter #24

Closed chadek closed 11 months ago

chadek commented 11 months ago

To be able to send any rawcmd and support the different buffer size implemented by all (at least many of them) version of this (weird?) protocol, I suggest to forget about the else if block and add a new parameter to be able to custom every command as needed.

Next step in an other PR would be to have some config map added to the config file and example for different implementation of the protocol.

Tested on axpert vm3 inverter this morning. I'm no cpp guru so I don't know if my parsing of the cli parameter is safe from shell injection or any of this kind of magic trick :p

A13xund3r commented 11 months ago

Why not just read until CR is received with max buffer set to something reasonable like 255 to trigger "response too long" error?

manio commented 11 months ago

@chadek Am I correct that it is trying to match those removed "else blocks" from the config file instead?

@A13xund3r may be right - maybe it could be auto-detected - but I am not sure if every command is adding this CR. Can this be verified? Sorry - I cannot test it currently on my real hardware :(

manio commented 11 months ago

Ah, got it - so this is the main feature and the documentation+config PR was for this. Merging then...

A13xund3r commented 11 months ago

@chadek Am I correct that it is trying to match those removed "else blocks" from the config file instead?

@A13xund3r may be right - maybe it could be auto-detected - but I am not sure if every command is adding this CR. Can this be verified? Sorry - I cannot test it currently on my real hardware :(

I've checked protocol PDF, every command and response is CR ended. In worst case of undocumented command it will trigger timeout. Also every response is checked for start/stop bytes so it is a "must have". And finally CR is used to match "message too short". It can be an issue for further parser as response length must match number of variables. Currently I'm checking response length for each command for PI30 protocol used by my inverter.

chadek commented 11 months ago

According to documentation, every command comes with a CR (which is good news because sometimes serial interface might fail). Yes I think response length could be auto-detected as @A13xund3r suggest. The only possible problem I see is if response sequence include a byte pattern that match the stop byte. In that case CR would fail but you won't know the real reason of that fail. You can imagine try to read the wall buffer until you get a valid stop byte and a CR matching but I think it is better to have something like a "scan" function that will try to map the length of queries and output them

A13xund3r commented 11 months ago

Finished comparing protocol 30 PDF with my inverter. 3 query commands (VERFW, QBOOT, QBATCD) are not supported returning NAK, response length to QMCHGCR and QMUCHGCR can be different for different inverters but it is always x*4+3 bytes. Response to QPIGS doesn't contain last 3 parameters which doesn't apply to OFF-GRID inverter. Response body is always printable ASCII char, no risk of CR but CRC can have it (if there is no case elimination in CRC calculation routine). I see 2 solutions:

  1. set 5ms timeout timer after CR reception, if there is no another byte received in that time then it is stop byte.
  2. after CR reception check message CRC is valid, if not then continue receiving data.
manio commented 11 months ago

@A13xund3r thanks for your investigation. Unfortunately as you probably can see I don't have free resources to maintain this project and add new functionality/protocol/new inverters. In fact it was only rather a "proof of concept" / demo application as the name is saying. I reworked this code in my hard project and I am using it daily, but only for my specific inverter. Anyway - I am open to any PR.