-
Notifications
You must be signed in to change notification settings - Fork 48
update pymodbus to new version to match home assistant (And additional code cleanup and fixes) #264
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
|
Hi. Thanks for this PR. I've tried it in Python 3.13 (HA core (mandatory next month)), but there are more things to change than just the import in coordinator.py. The actual version for pymodbus HA is 3.8.2 - and there are breaking changes in Pymodbus and in HA... I tried to debug that, without no success until now. |
|
@julesxxl works for me without any issues on 2025.1 beta with this fix. What kind of errors do you have? Maybe it's in a file I don't need because I don't have that Victron hardware. I only have a Multi GX and 2 mppt's Also pymodbus version used by HA is 3.7.4: https://github.com/home-assistant/core/blob/687afd23bcd3655da0fbdf8dc66161d99adb6658/requirements_all.txt#L2097 |
|
@remcom : Ah ok... strange... I've not kept the issues that I met. Are you on python3.13? (I own the Multiplus 2 GX with a Pylontech battery.) As soon as I can, I'll re-recheck and tell you. That would be great to clean the code too. Ruff, mypy et pylint find a lots of problems (but we'll can see that later). |
|
(By the way, I was testing on 2024.12.5) |
|
@julesxxl I'm on 3.13. It was running fine for me on 2024.12.x. I made these fixes only because of on 2025.1 HA switched to pymodbus 3.7.4. 2024.12.5 is still using pymodbus 3.6.x What kind of HA install do you run? It should al work fine when running recommended HA versions. (I'm running it on HA OS) If you have the error let me know. It's a while ago that i did python coding but i can give it a crack. |
|
I'm on the core version. A colleague of mine who uses Victron on HA OS have had no problem when 2024.12 came out. That's why I was surprised when I tried to switch from 3.12 to 3.13 et getting Victron not working anymore. I'll let you know. ;-) |
|
You tried to : from pymodbus.pdu import register_read_message, but register_read_message is not used in this file. And the type hint here won't work anymore : parse_register_data(self, buffer: ReadHoldingRegistersResponse... That's the first strange thing. |
|
I'm currently cleanup up the code some more and actually running in to this issue with ruff. Strange that it actually works on my HA machine. I will update the pr later today (little depending on all the christmas stuff). |
|
I've finally picked up homeassistant==2025.1.0b2. And it works! The changes you made are OK (but I had to import ReadHoldingRegistersResponse, as I wrote it)! Don't know what happened. I should have tried this HA version sooner, even if you had no problem with the 2024.12 ones. If you want, I can ruff/pylint/mypy if you think it's too hard. ruff --format will be easy, ruff check --fix a little less, but pylint and mypy show a very large amount of errors (that's horrific...). |
|
I've got all the Christmas stuff too, haha... (And I have to debug now my components Ecovacs, Invoxia and Ecovacs...) |
This reverts commit 98dc4bf.
|
I tried to fix it. I did some formatting cleanup but as soon as i want to add |
|
I actually have it correctly fixed now. locally i run 3.8 from pymodbus while HA is using 3.7.4 |
|
I'll check that! My job is to write in Python for cybersecurity. ;-) |
|
Nice. i just run pylint and indeed a lot of stuff! i can make a start with the cleaning up already |
|
I think the best way to be OK in the HA way is to take the conf in their pyproject.toml before running ruff, mypy, pylint |
|
I think i leave that up to you because i dont know what that is hahaha :P. If i can help or test with that let me know. I'm also curios how 'maintained' this repo is if i check open pr's and responses/merges on it |
…lint erros and fix PR sfstar#264 and fix issues sfstar#259, sfstar#261
|
OK. Picked up your PR changes + two issues + linters (but only ruff : mypy and pylint throw too many problems (I just fixed some of them). I've tested it in my HA instance: it still works. |
|
Please merge this PR, with HA v2025.1, the current release is broken |
|
I think this repo is not that actively maintained. I think if this not get actively merged soon I will make the for available in the hacs store |
|
Pls merge these changes. |
@sfstar please |
|
Hi all! Because this repo is maintained and i also like to use my Victron equipment in HA i made a new repo that you can use: https://github.com/remcom/hass-victron My goals:
Feel free to make your own pr's on the new branch or make issues on stuff you running into. Also if there are fellow maintainers that want to join please send me a message |
|
Closing in favor of #279 which was just merged. |
fix for: #263