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

Allow the serial port to be passed in so its open/close lifetime can be controlled #77

Merged
merged 9 commits into from
Aug 16, 2022

Conversation

schotime
Copy link
Contributor

There are situations where having the serial port be managed externally is really beneficial, such as sharing of said serial port on a bus.

This PR allows you to pass in an external ModbusRtuSerialPort such that the underlying SerialPort can be managed.

Let me know if you need changes, or I've missed something.
Great library!

@Apollo3zehn Apollo3zehn changed the base branch from master to dev August 16, 2022 06:36
@Apollo3zehn
Copy link
Owner

Thanks for your PR, this is a good improvement! I had to change your PR a little bit because in the Connect method you were closing the old port depending on if the current port is internal or not. There will be an old port if you call connect multiple times. That is why I moved the field to the ModbusRtuSerialPort class. I have also added some more doc strings.

I will merge the PR right now and make more changes in other parts of the library and then release the final v4 version so that is gets out of the preview status.

@Apollo3zehn Apollo3zehn merged commit aa039cb into Apollo3zehn:dev Aug 16, 2022
@schotime
Copy link
Contributor Author

Yeh, awesome. Thanks.
It might be good to have this for TCP (pass in the TcpClient) as well. Maybe less important if a modbus server accepts multiple tcp clients but still maybe worth having that option?

@Apollo3zehn
Copy link
Owner

I thought it was already possible but I confused it with the Modbus TCP Server. I will add that option tomorrow :-)

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.

2 participants