-
Notifications
You must be signed in to change notification settings - Fork 31
add ReadInput4 for registers 120-159 #239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This looks great. Ignore the tests which are probably going to fail, they're a pain and I'll probably just disable them to be honest. I'll merge this in today :) |
| Ok(ReadInput::ReadInput3(r3)) => { | ||
| let datalog = r3.datalog; | ||
| Ok(ReadInput::ReadInput3(r3)) => entry.set_read_input_3(r3), | ||
| Ok(ReadInput::ReadInput4(r4)) => { | ||
| let datalog = r4.datalog; | ||
|
|
||
| entry.set_read_input_3(r3); | ||
| entry.set_read_input_4(r4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think I've realised this change might break older inverters that don't send this 4th packet. Now those inverters will never trigger the all packet :(
This is an old bodge coming home to bite me, might have to have a think about how to fix this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually having that issue currently with my 18kpv where the All packet isn't being sent so all in mqtt isn't updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as a temporary workaround for this I'm considering a config option to tell lxp-bridge when to send the all packet; probably just publish_mqtt_all_after: 3 or something (then those who have a generator can set it to 4).
Longer term, I think I'd like to turn on publish_individual_input by default, then have HomeAssistant configuration watch for those instead, and ultimately retire the 1/2/3/4/all packets. They were a bit of an early poor design decision really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering how other inverters would handle the 4th as well.
@jgulick48 what version are you running on the 18k that doesn't have these? I'm on FAAB-1717 from January (not latest) - http://os.solarcloudsystem.com/#/releaseLog?deviceType=LXP_LB_8_12K
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on FAAB-1818. They are there, but for some reason the all packet doesn't get sent to mqtt unless I manually trigger a read input 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#249 underway with a new approach to packet decoding which should help with more than a few outstanding issues.

Generator data is available for EG4 18K on registers 120+. This adds a 4th ReadInput since only 40 registers can be read at once. Not much of 120-159 is populated on my EG4 18K so I only added what was populated that I could verify.
I've been reading v_gen, p_gen, f_gen for the last week (with AC coupled solar on the GEN port) into prometheus and the data looks correct.
EPS is after generator data but mine are all 0 so can't verify what those do - so left them out