Skip to content

Use non privileged tcp port in examples#109

Merged
OrangeTux merged 2 commits into
AdvancedClimateSystems:masterfrom
tiagocoutinho:examples-non-privileged-port
Nov 11, 2020
Merged

Use non privileged tcp port in examples#109
OrangeTux merged 2 commits into
AdvancedClimateSystems:masterfrom
tiagocoutinho:examples-non-privileged-port

Conversation

@tiagocoutinho

@tiagocoutinho tiagocoutinho commented Nov 7, 2020

Copy link
Copy Markdown
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

coveralls commented Nov 7, 2020 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

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

@OrangeTux

OrangeTux commented Nov 9, 2020

Copy link
Copy Markdown
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

tiagocoutinho commented Nov 9, 2020

Copy link
Copy Markdown
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

OrangeTux commented Nov 10, 2020

Copy link
Copy Markdown
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 force-pushed the examples-non-privileged-port branch from b7077cf to 792d785 Compare November 11, 2020 10:43
@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

tiagocoutinho commented Nov 11, 2020

Copy link
Copy Markdown
Contributor Author

I think it should be ready to review now

OrangeTux
OrangeTux reviewed Nov 11, 2020
View reviewed changes
Comment thread README.rst
# 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:

@OrangeTux OrangeTux Nov 11, 2020

Copy link
Copy Markdown
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

Reviewers

@OrangeTux OrangeTux OrangeTux approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiagocoutinho @coveralls @OrangeTux