-
Notifications
You must be signed in to change notification settings - Fork 17
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
Write battery parameters #2
Comments
Hi @hrford I'm glad to hear you like the library! The reason I hadn't got further with this feature is that I wasn't sure if the parameters have to be written in a block rather than one by one. Did you find this to be the case in your testing? I would be happy for you to work on a PR, I would support with review and testing on my controller as well 👍 |
Thanks! One question remains, if an API function call took the 12 required values. How would you like to do this? |
Your suggested API sounds very nice to use, I like it! It might be also good if there was an easy way to preview the initial values before setting anything (by calling the same function with no parameters?). |
minimalmodbus allows a read of multiple values (albeit without decimal point shifting). I propose three functions:
The user could learn the actual settings using get_battery_voltage_control_registers. set_battery_voltage_control_registers_dict would be a wrapper for set_battery_voltage_control_registers for users who wanted to pass in the dict obtained from get_battery_voltage_control_registers, otherwise, I expect most users just to use set_battery_voltage_control_registers The wrapper would be like: This would allow an incomplete dict to be used as get returns None when key isn't found. |
|
@rosswarren I have it running locally hence the import being tweaked. Not sure how you went about debugging. Perhaps let me know? |
@hrford I really like the way you implemented the feature. Sorry but I can't help test for a few days, as I don't have everything I need here right now. For debugging what I do is open a python shell in the directory of the code and run from epevermodbus import EpeverChargeController
controller = EpeverChargeController("/dev/ttyUSB0", 1)
controller.get_solar_voltage()
# etc. Is that what you meant? |
Thanks! |
Hey @hrford I just tested this functionality with my charge controller and it is working very well. I do have to sometimes call the function multiple times due to an error I see what you mean now about the command line tool, I can't get that to work either with the imports other than with the installed version. I am usually testing with only calling functions in the python shell directly. Thanks for the tip about ipython, I haven't used that! I am not normally a Python developer maybe you can tell 😄 |
Great, thanks for checking, I'll raise a PR and if anyone else finds bugs/improvements, then we can fix it in time. |
I'm working again on this project/branch. Will need to work on some of the Signed values to find which calls need it. I'm not sure about your issue with the comms, I've never had any failures, so perhaps you're experiencing a real hardware issue? The command line tool is kind of my test harness (I should build another). |
Tested some more and found a bug in another area... Are you ok if I raise issues more often? Would it be too noisy? Raised PR https://github.com/rosswarren/epevermodbus/pull/7/files which I couldn't work out how to link with this issue. |
Hey, really enjoyed finding your project in amongst a few others, (from a bit too scriptey, to a bit too objectey.)
I really enjoy the abstraction you've done and the command line utility really shows off how your implementation is straight forward.
In the readme, it's stated "Write battery parameters this feature is a work in progress".
I have a Tracer 2206AN and have already written my values for a 12V 100Ah battery. It seems I chose values close to those in your example.
I've raised this issue to cover adding that functionality. Would you mind if I added that functionality and created a PR for you?
The text was updated successfully, but these errors were encountered: