-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for typed device parameters #428
Conversation
This makes config files less verbose and means if/when we change the name of the main package, things won't break. Closes #403.
These accept e.g. a float rather than a list of possible parameters.
…evice_parameters()
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
+ Coverage 78.87% 79.04% +0.16%
==========================================
Files 56 56
Lines 2874 2925 +51
==========================================
+ Hits 2267 2312 +45
- Misses 607 613 +6 ☔ View full report in Codecov by Sentry. |
@CWestICL This feature could be used for the |
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 is really neat! Just a question, but nothing to change.
class SerialDevice( | ||
AbstractDevice, | ||
parameters={ | ||
"port": ( | ||
"USB port, including vendor ID, product ID and serial number", | ||
tuple(_get_usb_serial_ports().keys()), | ||
), | ||
"baudrate": ("Baud rate", BAUDRATES), | ||
}, | ||
): |
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.
I think you have mentioned this before, but I do not remember the answer:
This will retrieve the available USB ports that have "something" connected to them when this module is loaded, right? So, if the device of interest is off because we forgot to switch it on, then it will not be found. If we switch on the device, the collection of available USB ports will not be updated, so it will not be possible to connect to the device, anyway, the only solution being to restart the software.
Is this correct or am I misunderstanding anything? Nor that you need to change anything, just checking what the protocol is in that, very common, situation.
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.
Correct! This is the same as the existing behaviour and there is already an open issue for it: #335
I have an idea of how to fix it: the frontend could send a message to force a refresh of the COM ports before re-requesting the list of plugins. But it's more of a "could have" feature, so I haven't looked at it yet.
Add support for typed device parameters
This PR adds support for specifying arbitrary device parameters via a text box, rather than having to choose from a set list in a combo box (#377).
The changes to the frontend are fairly minimal. The device management dialog will now display text boxes or combo boxes for each of the parameters, depending on which is appropriate. While there are no labels saying what each parameter is, a description is provided in a tooltip so the user can find out by hovering.
The backend changes were a bit more invasive. I refactored the device plugin code, so now the user just has to specify the names of the parameters along with a description and, optionally, the list of possible values.
For example, the signature for the
EM27Sensors
class now looks like this:(Both of these are typed device params, i.e. their values can be arbitrary.)
The types and default values are automatically worked out from the constructor's signature:
If a child class changes/adds a default value for these params in its own constructor, then these will be used instead. This feature was added with
SerialDevice
s in mind: it now means that devices can specify a default baudrate in their constructors, without having to provide an extradefault_baudrate
class argument, as it was before.(I also implemented #403 at the same time.)
Closes #377. Closes #403.