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

Use non privileged tcp port in examples #109

Conversation

tiagocoutinho
Copy link
Contributor

It would be nice if the examples work out of the box without the user needing access to tcp port 502.

I propose using 15020 in this PR but I could change to something else if you prefer

@coveralls
Copy link

coveralls commented Nov 7, 2020

Coverage Status

Coverage increased (+0.5%) to 96.267% when pulling 88ea762 on tiagocoutinho:examples-non-privileged-port into 4fd98ba on AdvancedClimateSystems:master.

@OrangeTux
Copy link
Collaborator

Thanks for you suggestion!

I don't like to change to default port of the client, as Modbus server are usually listening to port 502.
Changing only the port of the server would make the example server incompatible with the example client. Therefore I also don't want to change the port of the example server.

But I realize it's not very friendly to just let the server crash. Therefore I suggest 2 thinks to make the server more user friendly, but keeping default port at 502.

  1. Catch the exception PermissionError, print a user friendly message to the user and exit.
  2. Add am option to modify the used port from the command line.

@tiagocoutinho
Copy link
Contributor Author

Sounds good!
Should the README examples be changed as well? Or, for the sake of readability, you prefer to keep them as is?

@OrangeTux
Copy link
Collaborator

Please update the README.md as well. I expect that a significant amount of users just copy the code from the README.md. Although I don't have proof to back it up

@tiagocoutinho tiagocoutinho force-pushed the examples-non-privileged-port branch 2 times, most recently from 65454a6 to b7077cf Compare November 11, 2020 10:39
@tiagocoutinho tiagocoutinho marked this pull request as draft November 11, 2020 10:50
@tiagocoutinho tiagocoutinho marked this pull request as ready for review November 11, 2020 10:55
@tiagocoutinho
Copy link
Contributor Author

I think it should be ready to review now

# Response depends on Modbus function code. This particular returns the
# amount of coils written, in this case it is.
response = tcp.send_message(message, sock)
with create_connection((host, port)) as sock:
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL that a socket.Socket object can be used as a context manager. Thanks!

@OrangeTux OrangeTux merged commit f1128a7 into AdvancedClimateSystems:master Nov 11, 2020
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

3 participants