-
Notifications
You must be signed in to change notification settings - Fork 781
Add Serial-over-IP (rfc2217) support in ESH Serial API #5560
Add Serial-over-IP (rfc2217) support in ESH Serial API #5560
Conversation
6ac8260
to
0c11583
Compare
@maggu2810: Please have a look it is an addition to your PR: #5313 I want to have an easy way in the framework itself to allow bindings to switch between serial port implementations. |
Could you elaborate on why you want to let the bindings to do the selection? In essence, the current serial API expects a single implementation to be present, while you are now asking to have the option to have multiple implementations being present at runtime. Imho the simplest (and non-intrusive) enhancement would be to leave it to the implementations to claim certain ports for them - e.g. any port that starts with @maggu2810 Any comments or ideas from your side? |
But then every implementation would need its own logic to decide which serial port to use AND - which is even worse - the implementation would need a dependency to the specific port (like TelnetSerialport which comes from nrjavaserial). |
Hm, for me the distributor should decide which implementations should be supported and not the user itself. |
I agree with @maggu2810 that the user should be the last one to be bothered with choosing a certain implementation. All that is asked from him is to specify the name of a serial port to connect to - our implementations are currently also supposed to provide a list of available names here. Imho it makes only sense to consider those "rfc2217" ports as serial ports, if they actually behave exactly the same, so from a binding perspective, it actually should not matter whether the port is local or remote - any existing binding that requires a serial connection should directly also work with remote ports; is this realistic? If so, the only way to distinguish between local and remote ports (and thus do proper handling in the serial implementation) is a naming convention wrt the port names. |
Exactly. Intention is that it makes no difference from the binding perspective whether it's a remote (rfc2217) or local serial port. Therefore the user can decide that (and is the only one that can do that because it's depending on his setup). Of course if the binding (or protocol) makes some timing critical things, it could happen that it is not working with rfc2217. |
Related to: #5560 Signed-off-by: Markus Rathgeb <[email protected]>
Please have a look at: https://github.com/eclipse/smarthome/compare/master...maggu2810:serial-port-provider-id?expand=1 If we assume that |
Imho this misses the point. What we need is a naming convention for "remote ports". Just like for local ports, it should not at all matter (to the user), which implementation is eventually used. @msteigenberger wants to add an implementation based on rxtx, but likewise, we could have other alternative implementations available that know how to remotely access telnet serial ports. |
If an user do not want to take care of the implementation, he just does not need to specify If the user want to force a special implementation, he can specify the So, e.g. if we don't want to differ between the specific implementation, but differ between types, we just need to change |
With the PR I linked above... ... there could be multiple serial port provider implementations be present at runtime and the user can choose which one to use or he can not care about. ... every serial port provider has an ID applied. This ID currently refers to the specific implementation of the provider for the serial port identifiers. We could also think about providing the type of the port (local, telnet com port), so the user can choose the type but do not need to care about the implementation. Should we support that the user can:
|
@maggu2810: I admit that your solution is very flexible, but it also adds some complexity to binding developers. IMHO we just have to consider following cases: Considering that we don't need to make any specific API for the binding developer.
Exactly. In my first thought implementation I did that but then moved to a less intrusive change for now. If we decide to go for the solution, I would change it.
Yes, we don't have a discovery mechanism for remote ports. That's a little drawback but Imho acceptable. |
@bodiroba told me about https://github.com/Tknika/serial2tcp-gateway where he is working on a daemon that watches the USB ports for known devices, creates TCP listen ports that relay to the physical serial port of that devices and advertises them on mDNS. Worked well in my short test. That can make the devices discoverable.
I agree on that, too. We could start by expecting only one implementation per provider (remote-port only uses rfc2217, local port only uses nrserial, ...). Thinking about the socket naming. I think that keeping the unix-style everything is a file system approach ( This would also allow the user to directly specify an implementation (a scheme) if he (or the binding) really insists on it by prepending the scheme like in said rfc1738 ( All of the URIs can be matched easily:
Show these regular expressions on regex101.com What do you think? |
Makes sense to me like that. Imho @gersilex suggestion looks pretty good - it would be fully backward compatible and "//" would tell us that we are talking about a remote port. If we agree that (for now) we leave it to the solution to choose the implementation, I would suggest to not support the optional scheme for now at all (as it only complicates the implementation and it can be easily added later without breaking anything).
Yes, but I am not sure how much that is gonna be used; we are talking here also about embedded devices that offer such a port, but where you cannot add any additional services yourself. So in any case, we will have to set @maggu2810 & @msteigenberger Would that solution be ok for you? If so, @msteigenberger, could you rework the PR accordingly? |
We already have the regular expression and we just need to expose the name in the specific implementation. I don't think it will complicate the handling. Or do I miss some point? If it is just a few lines I would prefer to add it as it will help to test serial provider implementations a lot. |
Well, you obviously need to be able to identify different implementations by ID (which influences the API), implement the selection logic, document what it is about, wonder whether you need end user documentation and what options you want to allow for solutions, add localisation and Paper UI support in case that solutions should be allowed to leave the selection of an implementation to the user, etc. That's why I simply think that as long as we do not have any requirement for it, better just completely leave it out for now... |
I've added a new commit.
|
Travis fails with:
|
@kaikreuzer: yes, thats because the current used version of nrjavaserial (3.12.0-OH) has no rfc2217 packages included. |
You formulated this nicely fuzzy. Digging into it, I realized that NeuronRobotics/nrjavaserial#109 still isn't merged, which is a prerequisite for us - and apart from this, it would be really time for a new official release of nrjavaserial... If no release is in sight, we would have to do a 3.13.0.OH build ourselves (but we would need somebody, who is capable of doing so...). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments, thanks for your effort
|
||
/** | ||
* | ||
* @author MatthiasS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be something like: forname surename - Initial contribution (and API)
* @author MatthiasS | ||
* | ||
*/ | ||
@Component(immediate = true, service = SerialPortRegistry.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need an immediate activiation?
} | ||
|
||
public Collection<SerialPortProvider> getPortCreators() { | ||
return Collections.unmodifiableCollection(portCreators); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unmodifiable collection is a view and so every change to the portCreators
will affect the user of the view.
This will potential lead to a CME if register or unregister is called while someone uses the view.
You should perhaps use something like Collection.unmodifiableCollection(new HashSet<>(portCreators))
.
*/ | ||
@Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC, policyOption = ReferencePolicyOption.GREEDY) | ||
protected void registerSerialPortCreator(SerialPortProvider creator) { | ||
this.portCreators.add(creator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you are using a dynamic reference the member portCreators will be accessed by different threads.
You could e.g. use a concurrent collection or other synchronization statements.
*/ | ||
@NonNullByDefault | ||
@Component | ||
public class SerialPortManagerImpl implements SerialPortManager { | ||
@Component(service = SerialPortProvider.class, immediate = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
immediate?
*/ | ||
@NonNullByDefault | ||
@Component | ||
public class SerialPortManagerImpl implements SerialPortManager { | ||
@Component(service = SerialPortProvider.class, immediate = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
immediate?
* | ||
* @author Markus Rathgeb - Initial contribution | ||
* @author MatthiasS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
|
||
/** | ||
* | ||
* @author MatthiasS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
|
||
/** | ||
* | ||
* @author MatthiasS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
* | ||
* @author MatthiasS | ||
* | ||
* @param <T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the interface does not contain a generic parameter
Good news, we should soon have a 3.14.0 release of nrjavaserial, see NeuronRobotics/nrjavaserial#109! |
I was just checking how we can upgrade the tp to nrjavaserial 3.14.0. We imho cannot add it to our ESH 3rd party p2 site as it is an LGPL "works-with" dependency. We cannot consume it directly from Maven Central as we need it on a p2 site. So I guess, I will have to best deploy a p2 site to the openhab bintray, just where we have the forked version right now as well. @maggu2810, @SJKA & @htreu Any better/other idea? |
|
Ha! Thanks for that. I’m away from home right now with just mobile phone access, so wanted to make sure I had as much information as possible to hand before I got back so I could hit the ground running! |
None of these hits is tested yet with rfc2217 support - in theory they should work, though. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Hi All, would this fix the issues I see when the console displays I/O errors because the virtual USB (mapped using ser2net & socat) has been removed or restarted? ie would the zwave binding be able to reconnect if that mapping was removed? Right now, as soon as the USB mapping is removed a restart of the Zwave binding is required. Sorry thats about as best I can explain the issue. More info here: https://community.openhab.org/t/share-z-wave-dongle-over-ip-usb-over-ip-using-ser2net-socat-guide/34895/85 Thank you! |
I don't remember there has been any change in this PR that is related to your point(s). |
Yes, because OH is no longer opening a serial port. There is no |
Thats great news! I hope this gets implemented in the stable release soon! |
Hi, I was about to go this way : https://community.openhab.org/t/share-z-wave-dongle-over-ip-usb-over-ip-using-ser2net-socat-guide/34895 because I need such a setup but if there is a more "mainstream openhab way" that is coming, I'd rather wait... Is it possible to have an rough view of when this will be available in openhab 2.4 branches i.e. within weeks or months or quarters? |
It is already available in latest snapshots of openhab. I tested it in my binding: |
Hi msteigenberger, can you tell me how/ what this changes? I'm a little confused over how this helps the issue or what is now different with the latest snapshot. I'm constantly experiencing issues with my zwave stick connected to a remote device using socat/s2net and when that device has a restart, OH2 looses connection to the usb stick and requires a restart of the zwave binding. |
@dastrix80: You don't need socat anymore that is located at the same machine as your OH2 instance. Instead OH2 now directly supports RFC2217 (which socat/ser2net is using). |
Which version of the snapshot was it added to? So I just change the Zwave binding to rfc://ip:port to make this work? |
I'm not sure about the z wave binding. From what I can see, it's not yet integrated. Sorry, didn't recognize that you talked about a specific binding. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/3rd-party-bluetooth-binding-beta-testers-needed/38492/738 |
I will try this new feature with the Sony projector I am developing. Regarding the binding, is it necessary to suppress the context serial-port from the thing configuration setting to let the user enter a value such as rfc2217://192.168.0.10:3333 ? |
// is possible and just means that it uses any remote implementation. As only rfc2217 is available, it would pick this. To be more precise you should rather use rfc2217:// You don't need to suppress the context in the configuration, as it would allow to enter manual values as well. |
Thank you for your answer. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/zwave-binding-updates/51080/662 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/preparation-for-2-5m2/75738/26 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
The actual instance of the SerialPort is chosen at runtime depending on the port's name.
This allows to have different implementations for different protocols. This can be e.g. RXTX (default local) or RFC2217.
Implementations are discovered at runtime via OSGi DS.
A client application now only needs to set the correct port name.
e.g.
Signed-off-by: Matthias Steigenberger [email protected]