we now have multiple devices that require a duration parameter for shaking on the firmware level:
therefore in v1 I propose including the duration in the capability backend
the nicest is front end calling start and stop, but unfortunately too many shakers do not support this on the firmer level. that means they wouldn’t be able to use the shaking capability
it means backends that do have start/stop on the firmware level will be responsible for sleeping. this is not ideal, but that’s how the standard will have to be given these devices. I prefer that over an ambiguous standard
Ah, you uncover more localities but then on the method level. I think it would not be wise to remove methods from more capable shakers because of architecture design. I can imagine you might want to shake until a temperature is reached, or shake until a procedure is finished.
So you might want to implement it with a Protocol alongside the ShakerBackend ABC just like the _CanLockInternally Protocol , but then CanStartStopShaking is not hidden.
good point, it’s not just about duplicating code (duration) but also we are actually losing functionality that is actually useful. i made the incorrect assumption that shaking was only for certain durations
if you propose making it a locality, is the idea to have two “things”
CanStartStopShaking, with people doing that thing through start and stop directly on the locality?this has a nice parallel to HasJoints for arms. But the difference is joints are always machine specific, whereas start/stop could be useful in “capability contexts”.
Good point on HasJoints, that’s a nice parallel.
I’d say one ShakingCapability that exposes everything. shake(speed, duration) as the universal standard, plus start_shaking() / stop_shaking() for backends that support it. The user goes through the same capability for both. More like the _CanLockInternally pattern, but user-facing instead of hidden.
When I said “locality on the method level” I meant the pattern; a method that’s not universal enough for the ShakerBackend ABC, like HasJoints is not universal enough for ArmBackend. The difference here is that start/stop is user-facing on the capability, while joints stay on the backend.
Two things would also be possible of course, a separate start/stop locality alongside the shaking capability. But one capability feels cleaner. Something like:
class ShakingCapability(Capability):
async def shake(self, speed, duration):
await self.backend.shake(speed, duration)
async def start_shaking(self, speed):
if not isinstance(self.backend, CanStartStopShaking):
raise NotImplementedError(
"This shaker requires a duration."
)
await self.backend.start_shaking(speed)
async def stop_shaking(self):
if not isinstance(self.backend, CanStartStopShaking):
raise NotImplementedError(
"This shaker requires a duration."
)
await self.backend.stop_shaking()
CanStartStopShaking can be protocol but can also be a mixin like HasJoints
I see, thanks for the explanation
I am not entirely sold on this pattern because capabilities were supposed to reduce the number of unexpected NotImplementedError errors. But maybe this case is unavoidable. The constraint is already coming from device’s firmware not being nice, so not necessarily a modeling error, but that is also something we can’t just ignore and blame on the firmware…
I think this might be a necessary evil, what do you think? probably better than just having it on the backend.
I think the NotImplementedError is fine here. It’s different from the old problem. Here, it’s a clear message to the user: “this shaker requires a duration, use shake(speed, duration).” The user discovers it at call time with an actionable message.
An alternative is to make the methods conditionally exist, only present when the backend supports them:
class ShakingCapability(Capability):
def __init__(self, backend: ShakerBackend):
super().__init__(backend=backend)
if isinstance(backend, CanStartStopShaking):
self.start_shaking = backend.start_shaking
self.stop_shaking = backend.stop_shaking
Same principle as star.iswap being None when not installed, the method is either there or it isn’t but type checkers won’t know about dynamically assigned methods, which could be real downside. Either way the constraint comes from the firmware, not from us. I think both are better than losing the feature entirely.
another question: is RPM actually the preferred unit or should we do hertz? Hertz seems more fundamental to me, but not sure what the convention is