Skip to content
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

Removed minimalmodbus constants, added default constructor arguments #10

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

limweichiang
Copy link
Contributor

Removed unsupported module-level constants for BAUDRATE and TIMEOUT, instead using default arguments in the constructor to initialize the values for use on the pyserial object.

This change took effect as of MinimalModbus 1.0 which the RenogyRover class inherits, refer to https://minimalmodbus.readthedocs.io/en/stable/usage.html#default-values

Before fixing, the following error was encountered:

$ python3 renogy_rover.py 
Traceback (most recent call last):
  File "renogy_rover.py", line 185, in <module>
    print('Model: ', rover.model())
  File "renogy_rover.py", line 41, in model
    return self.read_string(12, number_of_registers=8)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/minimalmodbus.py", line 751, in read_string
    return self._generic_command(
  File "/home/ubuntu/.local/lib/python3.8/site-packages/minimalmodbus.py", line 1170, in _generic_command
    payload_from_slave = self._perform_command(functioncode, payload_to_slave)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/minimalmodbus.py", line 1240, in _perform_command
    response = self._communicate(request, number_of_bytes_to_read)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/minimalmodbus.py", line 1406, in _communicate
    raise NoResponseError("No communication with the instrument (no answer)")
minimalmodbus.NoResponseError: No communication with the instrument (no answer)

Ubuntu and others added 2 commits October 24, 2020 12:17
"numberOfDecimals argument was changed to number_of_decimals as of MinimalModbus 1.0, refer to https://minimalmodbus.readthedocs.io/en/stable/apiminimalmodbus.html#minimalmodbus.Instrument.read_register
(1) Updates "numberOfRegisters" argument to "number_of_registers" in read_string()

(2) Moves unsupported module constants for initialization in constructor
@limweichiang
Copy link
Contributor Author

Sorry for the messy PRs, I'm kinda getting used to collaborating on code using git, and should keep my branches cleaner. Please do review #9 first, as the fix there should precede this.

Copy link
Owner

@corbinbs corbinbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@limweichiang thanks for these adjustments as well!

Like with #9 , I'll combine all of these adjustments and give them some runtime against my own 40A Rover before releasing a new library version.

I'd love to set up some automated tests for PRs here in the future. If you ever come across any good charge controller simulators, I'd love to try to 📈 some test coverage of the library to help expedite releases and improve charge controller coverage.

@corbinbs corbinbs merged commit e63d203 into corbinbs:master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants