Hi Lukasz, I just had a look and in general I like it. I have some comments, that I added which I'll sum up here:
1. I would opt for making the PlcConnection itself emit events ... having event emitting connections and non-emitting ones isn't nice. If we support that, all drivers should support it (Even if implemented in a trivial way) 2. While at it ... we shouldn't just emit connected and disconnected events, but could also emit if we identified a remote plc type and therefore Metadata changed. 3. I would opt for us changing all existing drivers to use the suppliers and get rid of the ones where we pass in a class. There's no advantage to us keeping both and refactoring the existing should be a thing of minutes. But in general, I like the changes ... and they definitely will help with passive mode drivers. As I was already asking myself: How will I be able to build the protocol-stack if I don't know the details of the connection ... I mean ... passive mode will have a significant impact on how the logic works. Also, for example, if we wanted to support Modbus via serial ... we would automatically have to switch to the hopefully upcoming ModbusRTU which uses a different entry type as ModbusTCP. So, I am in strong favor of your proposed changes, with these little remarks. Great job :-) Chris -----Ursprüngliche Nachricht----- Von: Łukasz Dywicki <[email protected]> Gesendet: Dienstag, 20. April 2021 02:03 An: [email protected] Betreff: Re: RFC / Tooling SPI for drivers / connection state tracking I have walking prototype, Here are code changes: https://github.com/apache/plc4x/pull/241 Because current project structure enforces separation of API and SPI I had to take some steps: 1) There is no way for user to know driver context, hence I touched public API. EventPlcConnection is added without extra metadata types since it is implementation feature not related to plc. Events can be changed to SPI where we will be able to provide better consistency. 2) End result of above separation is that we use API types instead of more specific ones in listeners. That's why any tooling will have to stick with it or do explicit class/interface casts. 3) I brought two ideas but in the end I merged them into one due to above. While tooling/audit could live in SPI, connection state tracking is something I would consider as part of API. I did try to separate "tooling" part without luck so far. Looking forward to hear your feedback. Best, Łukasz On 17.04.2021 19:12, Sebastian Rühl wrote: > Hi Lukasz, > > Answers below inline > >> At present we do not offer end user an API to react on connection >> shutdown or failure. There is no way for him to find that ie. link >> died and his program needs to take action. > > Sounds like we should add a EventListener so you could register for events > and get called back. > >> >> What I currently also do not understand/lack clearance is how stack >> configurer interacts with TCP connection. Here what bothers me: >> - Is there a new stack/decode instance per each active connection? >> - What is was intended design for driver context and why it is >> inaccessible via PLC connection interface? >> >> I suppose that stack per connection is desired case, but if that's >> true why we pass classes and instead of creating instances in stack >> configurer? > > We could add additional methods which accepts a lambda expression which in > turn gets the configuration as parameter. > This way you have the choice how you want to use it. > (Keep in mind that the StackConfigurer is an opinionated api and there > is no requirement to use it at any cost) > >> My case here is intended to attach connection state listener but also >> an message listeners which could help debugging message exchanges. >> All without tapping into wire. > > In general I think it would be helpful to tap into several places e.g.: > Just before the conversion to the protocol specific types Just before > sending the build protocol specific types onto the wire > >> >> A default way we advertise is making PCAP recordings which is fine >> for development. What I look for is an "logging" interface to be used >> in production or test environments without necessity to get libpcap >> involved. This would allow creation of additional tools if needed. > > Also this could be seen as an audit log 👍 > >> >> Best, >> Łukasz > > Sebastian > >
