Skip to content

RTU server#84

Merged
flosse merged 3 commits intoslowtec:masterfrom
bootrecords:master
Aug 16, 2021
Merged

RTU server#84
flosse merged 3 commits intoslowtec:masterfrom
bootrecords:master

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 flosse merged commit 84ddb16 into slowtec:master Aug 16, 2021
@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