Vspin driver improvements#1141
Conversation
|
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?
There was a problem hiding this comment.
This is just an educated guess.
Assuming there is no hardware difference and the distinction is purely firmware-based:
The "old" firmware is typically found on devices labeled "V11". It is highly unlikely that Agilent devices are running this outdated firmware, as they should all be on a newer version. Therefore, Agilent-labeled devices should work perfectly fine, whereas V11 devices might vary from one another.
The question is: how do we differentiate them? My suggested naming convention wasn't accurate enough.
To be honest, I'm not very familiar with that device, so I'm open to any good suggestions.
| 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
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.
TheRealDarkLuke
force-pushed
the
vspin-driver-improvements
branch
from
31ca42a to
6b7e907
Compare
| @staticmethod | ||
| def _find_status_packet(resp: bytes) -> Optional[_StatusPositionTachometer]: | ||
| for start in range(max(0, len(resp) - 13)): | ||
| packet = resp[start : start + 14] | ||
| if len(packet) < 14 or packet[0] not in _KNOWN_VSPIN_STATUSES: | ||
| continue | ||
| if (sum(packet[:-1]) & 0xFF) != packet[-1]: | ||
| continue | ||
| return VSpinBackend._StatusPositionTachometer.from_buffer_copy(packet) | ||
| return None |
There was a problem hiding this comment.
this seems a bit fragile as it silently skips "unknown" vspin status responses. How do we know we have all? There has to be some better way of reading status responses. I think they arrive in individual ftdi packets
| def _with_vspin_checksum(cmd: bytes) -> bytes: | ||
| """Return ``cmd`` with the final VSpin checksum byte recomputed.""" | ||
| if len(cmd) <= 2 or cmd[0] != 0xAA: | ||
| return cmd | ||
| payload = cmd[1:-1] | ||
| return b"\xaa" + payload + bytes([sum(payload) & 0xFF]) |
There was a problem hiding this comment.
this strips the first byte and the checksum from the given command, and then adds it back. In the case of the first byte, it is always 0xAA per previous if statement, so removing that is always unnecessary. But I think what we should really do is store the "stem" of each command and have the checksum and 0xAA added automatically
3 participants
Add a driver for the old V11 Vspin