Vspin driver improvements#1141
Conversation
Keep the existing Agilent VSpin backend as the default path for newer Agilent-branded centrifuges. Add a separate Velocity11 legacy backend for the trace-derived V11 communication sequence and expose create_vspin_backend(..., variant='agilent'|'v11') for explicit selection. Note: hardware validation for the legacy V11 path is still pending; these changes are untested on real Velocity11 hardware.
Add a pyserial-backed FTDI path for Windows COM-port access, harden the V11 startup and runtime attach flow, and make homing wait for real controller activity before accepting idle status. Validated with the local VSpin hardware workflow at 300 g for 10 seconds.
Reject all-zero runtime attach status packets and fall back to full 14-byte status polling when short idle status lacks position/home data during homing. Validated with the local VSpin hardware workflow at 300 g for 10 seconds.
Do not accept a zero home position after homing, refresh bucket targets when the home reference is unknown, and require measured RPM before considering a spin cycle started. Validated with the local VSpin hardware workflow at 300 g for 10 seconds.
… fallback to quick-patch-v11)
Removed 'pyserial' from the 'ftdi' optional dependency list.
|
thanks for the PR! interestingly, we have vspins with the Agilent and v11 label and both of them work through the existing backend. I wonder if this is not an agilent/v11 difference but rather a firmware version difference which would correlate with agilent/v11 (because Agilent acquired them of course) the structure of the code looks very similar between the two. Rather than having a completely new backend, why dont we have one backend with a structure like this? def something():
if self._model == "agilent":
cmd = "A"
elif self._model == "v11":
cmd = "V"
else:
raise
self.send(cmd)this way we have less duplication and the "Agilent" version can benefit from your improvements as well. It might also give us some more clarity on what exactly changed between versions and if there's an even smaller abstraction |
|
Good catch! I didn't think of that, but it makes perfect sense. Fair point about the duplicates—I'll fix that right away. |
rickwierenga
left a comment
rickwierenga
left a comment
There was a problem hiding this comment.
thanks for the refactor!
looks like we actually did have this "old" firmware at some point, and then I changed it to this new format to make it work on some other vspins and since it worked on others must have assumed this new one was more universal. That was clearly a mistake, we have too many vspins 😅
still the question stands what is the difference between them? why do you think it's Agilent vs v11?
| class _FakeIO: | ||
| def __init__(self, read_chunks): | ||
| self.read_chunks = list(read_chunks) | ||
| self.writes = [] | ||
|
|
||
| async def read(self, num_bytes: int) -> bytes: | ||
| if self.read_chunks: | ||
| return self.read_chunks.pop(0) | ||
| return b"" | ||
|
|
||
| async def write(self, data: bytes) -> int: | ||
| self.writes.append(data) | ||
| return len(data) |
There was a problem hiding this comment.
why not use unit test mock for this?
| def _command(self, name: str) -> bytes: | ||
| return _VSPIN_COMMANDS[self._command_set][name] | ||
|
|
There was a problem hiding this comment.
this name is a bit confusing. maybe _get_command_bytes or sth?
| def test_find_status_packet_scans_noise_and_validates_checksum(self): | ||
| packet = _status_packet() | ||
|
|
||
| parsed = VSpinBackend._find_status_packet(b"\x00\xff" + packet + b"\x00") | ||
|
|
||
| assert parsed is not None | ||
| self.assertEqual(parsed.status, 0x11) | ||
| self.assertEqual(parsed.current_position, 12070) | ||
| self.assertEqual(parsed.tachometer, -10) | ||
| self.assertEqual(parsed.home_position, 6733) | ||
|
|
There was a problem hiding this comment.
this depends on the default parameter in _status_packet. ideally we would just test parsing
3 participants
Add a driver for the old V11 Vspin