-
-
Notifications
You must be signed in to change notification settings - Fork 21
Several medium sized improvements #92
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
jaredmauch
commented
Jun 20, 2025
- add json output to ease testing
- add influxdb v1 support for ease of use with grafana
- improve eg4 handling
* add json output to ease testing * add influxdb v1 support for ease of use with grafana
very nice. |
config.influxdb.example
Outdated
@@ -0,0 +1,22 @@ | |||
[influxdb_output] | |||
type = influxdb_out |
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.
need to verify "type" is an alias of "transport"
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.
Oh, no religion here, change at will :-)
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.
classes/protocol_settings.py
Outdated
@@ -626,7 +626,7 @@ def process_row(row): | |||
concatenate_registers.append(i) | |||
|
|||
if concatenate_registers: | |||
r = range(len(concatenate_registers)) | |||
r = range(1) # Only create one entry for concatenated variables |
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.
hmm. i dont remember enough to know if this is ok... :P
return results[entry.variable_name] | ||
|
||
# Special handling for concatenated ASCII variables (like serial numbers) | ||
if entry.concatenate and entry.data_type == Data_Type.ASCII: |
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 not sure why this change is needed?
is there a byte order or encoding issue with eg4 devices?
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 it returns a word and you need to split it into the two bytes low and high. In the CSV this is done, but maybe the Serial Number thing in the concat isn't quite right as it needs to concat the high/low bytes? Recommendations of better ways to do this are welcome. The intent is to have multiple inverters going to my influxdb and then can match on the serial of the inverter so if the usb/rs485 cables are shuffled at some point the actual inverter serial is reported as the tag to influxdb/json/whatnot for eg4 - have the 12kpv and 18kpv inverters I'm wiring to this
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.
oh ok. yeah it's just how it's done in the csv.
by default, i just do it the same way that it's documented. the eg4 one isnt one that i've personally tested.
this could easily be changed to:
ASCII 115~119
alternatively the serial number can be manually configured via cfg.
if you can send me a dump of the input and holding registers, that would allow me to emulate and test things in detail.
analyze_protocol = true
analyze_protocol_save_load = true
run once and exit.
should create two files: input_registry.json, holding_registry.json
then i can use that to test in detail.
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.
4db8362
v1.1.10 branch
should theorectically do the trick...
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 also needed to do this for fwcode as well, otherwise it was in the wrong byte order, so I think this was needed. Let me get you what you are asking for though :-)
your doing well with navigating through ppg :D the new transports look ok. but the concatination changes + ascii handling seems a bit off. that stuff is done here:
|
- Fix AttributeError when accessing ModbusIOException.error_code in pymodbus 3.7+ - Simplify modbus transport compatibility to only support pymodbus 3.7+ - Remove unnecessary pymodbus 2.x compatibility code - Fix client initialization order issues in modbus_rtu and modbus_tcp - Add safety checks for addresses list initialization - Update error handling to work with newer pymodbus exception structure This resolves issues when analyze_protocol=true and improves compatibility with modern pymodbus versions.
- Fix UnboundLocalError when ModbusIOException occurs during register reading - Initialize register variable before try block to prevent undefined access - Add safety checks for register.registers access when register is None - Improve enable_write validation with exception handling and retry logic - Add delay before validation to ensure device is ready during initialization - Better error handling for validation failures during analyze_protocol mode This resolves issues when analyze_protocol=true causes validation to fail due to device not being ready or connection issues.
- Skip init_after_connect validation when analyze_protocol is enabled - Prevents validation from running during analyze_protocol initialization - Fixes timing issue where validation was called before client was fully ready - Maintains normal validation behavior when analyze_protocol is false This resolves the core issue where analyze_protocol=true caused validation to fail due to premature execution during initialization.
- Use configured protocol's register ranges instead of maximum from all protocols - Prevents trying to read non-existent registers from other protocols - Fixes 'No response received after 3 retries' error when analyze_protocol=true - Uses the actual configured protocol (eg4_v58) instead of creating new protocol objects - Maintains backward compatibility with fallback to original behavior This resolves the core issue where analyze_protocol was trying to read registers 0-1599 when the actual device only has registers 0-233.
I emailed you (i hope) in private some notes on this. There were some bugs when analyze_protocol=true is set - it would not honor the configured baud rate (19200) and used a default of 9600. There's a lot of individual commits as I developed on a local machine and tested on a remote that has the hardware on it. I'll try out your csv change as well and see if that works as desired. |
cool. got your email; was in the spam folder :P didn't have to go so far with the analyze protocol; just the dump. got the dump! i'll look at things in detail hopefully later today. |
Yeah, debugging the serial port speed thing was a fun side-quest of the main side-quest. my primary goal of the serial number stuff is to have the data in influxdb have the serial number promoted out of the individual record to the device_serial_number making it easier to grab comparable values across multiple inverters and observe any oddities. Our electric company here is regularly giving us power that is outside of the spec that is acceptable. I can split this into different PRs if that's easier, but this did kinda spiral out. I can get you into a device with the inverter attached as well if that helps at all. |
Well it's not quite working right with my most recent change, looking at it as influxdb has:
while the column with the serial has:
which is the correct value |
ok, looks like the "register" order is correct, but the byte order is incorrect for the eg4_v58. |
alright, i've put in a clean solution to the problem in v1.1.10 the byteorder can be specified on a per register level... to help fight those bad chinese programmers :P canbus has better support for byteorder, other datatypes \w modbus will need to be updated to account for byteorder in the future. |