-
Notifications
You must be signed in to change notification settings - Fork 167
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
feat: add remote modbus rtu driver support, refactor ModbusRTUDriver to use SerialPort directly #1394
base: master
Are you sure you want to change the base?
Conversation
|
||
@target_factory.reg_resource | ||
@attr.s(eq=False) | ||
class ModbusRTU(SerialPort, Resource): |
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.
This resource should still be available, issue a deprecation warning and use the new resource automatically.
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.
Of course, this should be fixed. I also adjusted the commit messages to (hopefully) match the style used in labgrid
3df93ca
to
7c00b6c
Compare
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.
Looking at the minimalmodbus documentation, it seems to support communicating with multiple instruments on the same bus by sharing a pyserial object, but with the ModbusRTUDriver we can bind only one device to the serial port resource.
Wouldn't it make more sense to have drivers for each instrument bind to the SerialDriver
and pass the SerialDriver.serial
attribute to the minimalmodbus Instrument
constructor?
To allow multiple client drivers binding to a SerialDriver instance, you'd need to override the resolve_conflicts
method from the ConsoleExpectMixin
and allow multiple active clients only if they are all instrument drivers (and then call super()
).
Going this route would also avoid the backwards compat problem, as the new driver would be a ModbusRTUInstrumentDriver
.
self.instrument = self._modbus.Instrument( | ||
serial_if, | ||
slaveaddress=self.address, | ||
close_port_after_each_call=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.
Why do you use close_port_after_each_call=True
?
@b2vn As you contributed the original Modbus RTU support in #806, do you have an opinion regarding this refactoring and my suggested approach in #1394 (review)? |
refactor ModbusRTUDriver to use SerialPort resource directly Signed-off-by: Felix Zwettler <[email protected]>
ensuring backwards compatibility Signed-off-by: Felix Zwettler <[email protected]>
Signed-off-by: Felix Zwettler <[email protected]>
aa82ee7
to
be1a228
Compare
Description
This PR adds support for using Modbus RTU serial ports remotely. It does that by removing the ModbusRTU resource, and let the driver use
SerialPort
andNetworkSerialPort
resources directly.In order to archieve this, I moved the fields
timeout
andaddress
into the driver.My reasoning is that it makes more sense to let the resource manage the entire bus, and let the driver only control one slave (= address). Same for the timeout, this can be slave-specific.
It seems to be working fine remotely with a Waveshare RTU relay, so far I only tested it with a low baudrate 9600 so I don't know how well the remote serial connection behaves with higher rates.
as mentioned in #1370 I am unsure if it is "okay" to replace the ModbusRTU resource, because this is obviously a breaking change.
Adding a exported NetworkModbusRTU resource would be the alternative, but it would mean a lot of repetition with NetworkSerialPort. But I might have missed intentional architectural decisions here, so I can implement whatever makes the most sense!
Once this is resolved/merged, I'll follow up with an added
WaveshareRTURelay
driverChecklist