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

RTU server #84

Merged
merged 3 commits into from Aug 16, 2021
Merged

RTU server #84

merged 3 commits into from Aug 16, 2021

Conversation

bootrecords
Copy link
Contributor

@bootrecords bootrecords commented Aug 2, 2021

Adds RTU server functionality based on #72

This PR simply replicates that previous proposal that was sidelined by the #78 merge, but updated to compile based on the changes in tokio-serial.

Functionality has absolutely not been field-tested by me (beyond the provided example code), and since I have no practical use-case for an RTU server application, I'd ask someone more involved in this field (like @efancier-cn @ivomurrell or @david-mcgillicuddy-moixa) to do the necessary validations.

Closes #73

@bootrecords bootrecords changed the title added RTU server functionality based on #72 RTU server Aug 2, 2021
@david-mcgillicuddy-moixa
Copy link

david-mcgillicuddy-moixa commented Aug 2, 2021

Thanks for the PR, I'll take a look when I can (hopefully later this week) and test it with our whole stack and hardware.
edit: I've not forgotten, first thing next week!
edit 2: working on it as we speak

@uklotzde
Copy link
Member

uklotzde commented Aug 13, 2021

Include in v0.5.0? If yes then we should add this PR to the milestone.

@uklotzde uklotzde requested a review from flosse August 13, 2021 12:15
@uklotzde
Copy link
Member

Please rebase on master and force push to enable the new CI workflow tasks.

@bootrecords
Copy link
Contributor Author

Hm, looks like the SerialStream::pair() function used in one example is only defined on Unix. I'll look into fixes for that later

@uklotzde uklotzde added this to the v0.5.0 milestone Aug 13, 2021
@uklotzde uklotzde mentioned this pull request Aug 13, 2021
14 tasks
@david-mcgillicuddy-moixa
Copy link

david-mcgillicuddy-moixa commented Aug 13, 2021

Hm, looks like the SerialStream::pair() function used in one example is only defined on Unix. I'll look into fixes for that later

Unfortunately even all the way down into serialport::COMPort (which backs tokio-serial and mio-serial's SerialStream types on windows) there's no pair function: https://gitlab.com/susurrus/serialport-rs/-/blob/master/src/windows/com.rs#L37

e: as for my testing I ran into https://gitlab.com/susurrus/serialport-rs/-/issues/105 so I'm going to keep trying when I get a chance today

@bootrecords
Copy link
Contributor Author

bootrecords commented Aug 13, 2021

Well, in that case I see two main options: Either enclosing that example in #[cfg(target_family = "unix")] and putting some placeholder for the not case, or changing the example to use safe constructors, but then it becomes dependent on connected hardware (not unlike the other RTU examples, actually) whether it would actually run successfully.

EDIT: on second thought, there may be a hybrid approach in which we simply use the pair() constructor on #[cfg(target_family = "unix")] and a regular open() otherwise

@david-mcgillicuddy-moixa
Copy link

david-mcgillicuddy-moixa commented Aug 13, 2021

It'll at least compile on windows now, but it seems unlikely to me that this example could possibly work on windows due to "/dev/ttyUSB0" but maybe that's just the best we can do really.
e: somebody running this on windows will have to just edit it for the right com port for the connected device, as you said.

@bootrecords
Copy link
Contributor Author

I was thinking the same thing... that's the trouble with most of the examples (at least on the rtu feature) at the moment, though. Somebody may want to spice those examples up with windows/unix cfg dependent addresses at some point, perhaps.

@david-mcgillicuddy-moixa
Copy link

david-mcgillicuddy-moixa commented Aug 13, 2021

Note that even on a unix platform, namely OSX, I still get the IOCTL errors from pair giving me a virtual tty (see the above linked issue 105), so bear in mind that isn't a perfect solution either. I'm happy enough with this solution for now although yes it would be great if one day all the examples ran over a virtual pair and so worked independently.

@flosse
Copy link
Member

flosse commented Aug 16, 2021

I'm sorry to bug you again but would you mind to rebase this branch again? Otherwise if I'd do it GitHub would not recognize this PR to be merged :-\

@flosse
Copy link
Member

flosse commented Aug 16, 2021

thx!

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.

add rtu to server
4 participants