-
Notifications
You must be signed in to change notification settings - Fork 121
BioShake backend #711
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
BioShake backend #711
Conversation
A new backend for the BioShake, as well as adding the class to the __init__.py file
| HAS_SERIAL = False | ||
| _SERIAL_IMPORT_ERROR = e | ||
|
|
||
| class BioShake(HeaterShakerBackend): |
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.
Do you know the different BioShake models this would likely work for?
If we notice a difference in commands between different BioShakes I'd recommend specifying the class name further: e.g. BioShakeQ1Backend and adding the known catalgoue/order numbers of the machines that we know can be controlled with this backend into the class' docstring.
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 integration manual I was referencing says that these commands are for: BioShake 3000, BioShake 3000 elm, BioShake 3000 elm DWP, BioShake D30 elm, BioShake 5000 elm, BioShake 3000-T, BioShake 3000-T elm, BioShake D30-T elm, Heatplate, and ColdPlate.
I tested my script on the BioShake 3000 elm, BioShake 5000 elm, and BioShake D30-T elm, and can confirm that the firmware codes are the same.
As for the Q1 model, even though I do not have that, the old integration manual shares the same code as the newer one I am referencing. Therefore, it should work the same. The only difference between them is the physical dimension of the device, obviously.
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 this is good to keep for now (since it seems the firmware interface is for all BioShake models), maybe as camillo said put a comment describing which models you tested this on. In the future we can always refactor/extend the code to support more models. If we want to support the ColdPlate/HeatPlate separately they could share a base class. Future stuff
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.
That is true, even for the HeatPlate and ColdPlate, I don't know what else users want to control besides starting, getting, and stopping temp control with the current codes I have. Either way, I can prepare a table in the documentation page on the PLR Wiki for the BioShake, where I list the models and what features each supports (shaking, plate lock, heating, active cooling), and what models were already tested with the current script.
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.
table sounds great!
1.) Add reset and home in setup 2.) Add acceleration/deceleration to shaking function 3.) Asserted speed, temperature, and acceleration based on model 4.) More error handling in general
|
Also when I |
my bad... there was a bad version of main (because I didn't format before pushing there) that happens to be the version you made the branch from. it will be fixed when merging main back into this branch because I made the same fix on main. it will resolve automatically. sorry about that |
1675cd6 to
2b2f7f9
Compare
Fix parameter of shake and stop_shaking functions
| min_temp <= temperature <= max_temp | ||
| ), f"Temperature {temperature} C is out of range. Allowed range is {min_temp}–{max_temp} C." | ||
|
|
||
| temperature = temperature * 10 |
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 we wrap this into a round(temperature * 10) ?
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.
we are doing that here: https://github.com/PyLabRobot/pylabrobot/pull/711/files#diff-fb7fb73412de1d7d3cb3f2130f5a897e1ed89dd672ca8034ec24fb53c0ed1310R233 (albeit with an int conversion not round), after checking that it's a whole number. I suppose the error message could be improved, do you think it's necessary?
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.
Nice - yes, I think we should do it now while we still remember :)
A new backend for the BioShake, as well as adding the class to the init.py file. Let me know if anything else is needed or revised.