Prior stage with abstract stage class #65
Conversation
| self.joystick_enabled = False | ||
|
|
||
| # Prior-specific properties | ||
| self.stage_microsteps_per_mm = 100000 # Stage property |
There was a problem hiding this comment.
These seem like properties of the stage that'd be defined in StageConfig (everything from microsteps_per_mm to acceleration) - we have a lot of them in the existing StageConfig.
Instead of hard coding them here, does it make sense to use the existing StageConfig mechanism?
It may be that the PriorStage only has 1 valid config, in which case we could define the instance in this file or as an attribute on the class, but we should still use the existing StageConfig mechanism instead of re-creating it here.
You might need to modify the type signature of StageConfig to allow some of the fields to be Optional if PriorStage doesn't support/need all of them - that's okay to do. It'd be good to add unit tests to for those cases. For instance, adding a test to make sure we can create instances of a config for the CephlaStage and the PriorStage.
For example, you can take a look at tests/squid/test_config.py. We'd want to modify the test there to:
- make it so
squid.config.get_stage_config()can be asked to give us a config for theCephlaStageor thePriorStage. - Run that test for both the prior and cephla stage configs
Then we can use the PriorStage variant of the config here (and eliminate all the config-like attributes here).
| @@ -29,6 +27,10 @@ | |||
|
|
|||
| import control.filterwheel as filterwheel | |||
|
|
|||
| if USE_PRIOR_STAGE: | |||
There was a problem hiding this comment.
Instead of doing this if with hidden imports here and if for stage creation below, I think what we should do is:
import squid.stagehere- in
squid.stage.__init__.py, add aget_stage(stage_config: StageConfig). - If we don't have it already, add a
stage_type: StageTypefield toStageConfigso we can define what stage type we're using. Add aclass StageType(enum.Enum)if we don't have it!
The reasons I'd suggest this are:
- We don't want users of the
AbstractStageconcept to need to care about how/when to build the different stage variants - we want to hide that from them! That's the reason for having anAbstractStageconcept. Users only need to be aware of theAbstractStageand the interface it represents, and then our software can take care of giving them the correct instance under the hood. - Since it's going to be common for users to create a stage (eg: if they're writing a headless script), we don't want this logic to need to be repeated in each of those cases. If we instead implement it once in a
squid.stage.get_stage(...), then it won't need to be repeated every time someone wants to create a stage.
| @@ -38,6 +38,15 @@ def _configure_axis(self, microcontroller_axis_number: int, axis_config: AxisCon | |||
| ) | |||
| self._microcontroller.turn_on_stage_pid(microcontroller_axis_number) | |||
|
|
|||
| def x_mm_to_usteps(self, mm: float): | |||
There was a problem hiding this comment.
If we want to add these (we may not want to since usteps is a concept that only makes sense when using stepper motors. If we ever have stages that don't use steppers it'll be confusing), we should add all 3 to the AbstractStage interface as abstract methods.
The way to think about AbstractStage is that it is the only structure the rest of our system can know about. It defines the interface between all other parts of the system, and stages.
This is important, because it means the rest of the system and users of the system only need to care about how AbstractStage is defined, and don't need to know any the nitty gritty details of how the individual Stage implementations work.
To keep this abstraction, though, we need to make sure that we add methods to AbstractStage if they're going to be used elsewhere in the system.
Also it's generally a good idea to "keep your interfaces as small as possible" -- don't add methods/attributes/etc if you don't need to -- because these interfaces are what we need to support and maintain. The smaller the surface area we need to maintain, the better.
| def set_baudrate(self, baud: int): | ||
| allowed_baudrates = {9600: "96", 19200: "19", 38400: "38", 115200: "115"} | ||
| if baud not in allowed_baudrates: | ||
| print("Baudrate not allowed. Setting baudrate to 9600") |
There was a problem hiding this comment.
I think instead of setting a baud that wasn't asked for here, it'd probably make sense to throw a ValueError exception. It's likely that if someone is giving an invalid baudrate here, they are confused or the system isn't configured properly (and so it's safer to fail than to continue).
| baud_command = "BAUD 96" | ||
| else: | ||
| baud_command = "BAUD " + allowed_baudrates[baud] | ||
| print(baud_command) |
There was a problem hiding this comment.
It'd be good to start using our logging mechanism instead of print. To do that, you should:
- In the
__init__of your class, define aself._log = squid.logging.get_logger(self.__class__.__name__) - In your class, instead of
print, useself._log.*. - For errors, use
self._log.error - For important informative messages that won't be frequent and too annoying in the console, use
self._log.info - For recoverable errors that you can continue after, use
self._log.warning - For low level debugging information that is too verbose to always print out, use
self._log.debug
This is good to do because it lets us adjust the logging level (which isn't possible to do with print), and it lets us send log messages to a bunch of different locations. For instance, in squid.logging.add_file_logging we have the mechanism for log messages to both go to the console and a file at the same time (so we have a record of the logs that aren't just in the console).
| raise ValueError("Acceleration must be between 1 and 1000") | ||
|
|
||
| def enable_joystick(self): | ||
| self._send_command("J") |
There was a problem hiding this comment.
Is there a way we can check for a success response here? I think if no bytes come in after the timeout, this will still return and we'll assume all is good.
There was a problem hiding this comment.
Same below and anywhere the return value of _send_command is ignored.
| print(f"Current acceleration: {response}") | ||
| return int(response) | ||
|
|
||
| def _mm_to_steps(self, mm: float): |
There was a problem hiding this comment.
If we use config, I think the AxisConfig already has helpers for this? I might be missing something, though!
| self.wait_for_stop() | ||
| else: | ||
| threading.Thread(target=self.wait_for_stop, daemon=True).start() | ||
|
|
||
| def move_z(self, rel_mm: float, blocking: bool = True): |
There was a problem hiding this comment.
Since the Prior doesn't have a Z axis, should this throw a NotImplementedError or something? Since it might not be valid for someone to ask the Z axis to move.
I imagine this could happen accidentally if someone tried to do a z stack acquisition, or something else like that. We'd rather the acquisition fail than proceed quietly without the Z movement happening.
| if blocking: | ||
| self.wait_for_stop() | ||
| else: | ||
| threading.Thread(target=self.wait_for_stop, daemon=True).start() |
There was a problem hiding this comment.
Is kicking off a thread per request a good idea here? I think in the other implementation, we assume that a user using blocking = False knows what they're doing, and so won't issue more commands until the stage isn't busy anymore.
If the worry is that we need to make sure to clear the response from the serial queue, we might need a background thread that's always running.
As is, if someone does a :
self.move_y(..., blocking=False)
self.move_x(..., blocking=False)
self.move_y(..., blocking=False)
we'd immediately kick off 3 threads!
| self.y_pos = 0 | ||
|
|
||
| def wait_for_stop(self): | ||
| while True: |
There was a problem hiding this comment.
We want to be careful not to have potential for "infinite hangs" in our code, so you should add a timeout argument here and have the while loop throw a TimeoutError.
No description provided.