-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(api): Add locks on direct data access for mag and temp module #4501
Conversation
- Remove backcompat wrapper tests since we have no backcompat wrapper now - Remove V1 json protocol execution since it is pointless now - Fix our test fixtures to not consider the v1/v2 split for anything that - doesn't really need it - Fix our tests to deal with the new test fixtures Upshot: tests run faster now, and also we can test apiv2 deck cal cli for the first time.
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 changes to make the magdeck get its information from the hardware look good. The locking is a bit wrong though.
What we want to do with the locking is add it at the highest atomic layer we can - at the place where you can't have any interruptions. That place is in driver send/receive. We want to make sure that the driver holds the lock from before it starts sending to after it's done receiving, including any retries in the middle. Those places aren't unfortunately all going through the same paths in the driver - for instance, magdeck recursive_write_and_return
vs raw _send_command
- but all those places are where they need to be.
Finally, the locks actually need to be in a place where multiple things can contend for them. That means that each driver file should have a module-scope map of module port to lock, and individual driver instances acquire the lock that corresponds to their port. The way it is now, with each module instance having its own lock, protects against multiple threads accessing a specific module:
thread-1 thread-2
td.set_temperature()
-> acquire td.set_temperature()
-> write -> acquire waits....
-> read ...
-> release acquires
returns -> write
but it doesn't protect against multiple module instances in different threads:
thread-1, td-1 thread-2, td-2
set_temperature
-> acquires lock-1 set_temperature
-> writes -> acquires lock-2
-> reads -> writes # possible collision!
which is the thing we're most concerned about, since we'll have a v1 module object and v2 module object talking to the same hardware.
Codecov Report
@@ Coverage Diff @@
## edge #4501 +/- ##
=========================================
+ Coverage 55.46% 57.4% +1.93%
=========================================
Files 909 917 +8
Lines 26743 28482 +1739
=========================================
+ Hits 14834 16350 +1516
- Misses 11909 12132 +223
Continue to review full report at Codecov.
|
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.
Couple minor things but code lgtm and let's merge once tested
if not lock_for_port: | ||
mag_locks[port] = Lock() | ||
lock_for_port = mag_locks.get(port) | ||
self._lock = lock_for_port |
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.
Probably want to look this up from the global every time unfortunately
@@ -46,6 +46,8 @@ | |||
# to/from Temp-Deck | |||
GCODE_ROUNDING_PRECISION = 3 | |||
|
|||
mag_locks: Dict[str, object] = {} |
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.
this can be Dict[str, threading.Lock]
right
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.
true
@@ -68,8 +68,10 @@ def connect(self, port=None) -> Optional[str]: | |||
self.disconnect(port) | |||
self._connect_to_port(port) | |||
if temp_locks.get(port): | |||
print("in here") |
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.
Remove the print statements in line 71 & 74
Everything else looks good to me!!
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.
Tested with V1 and V2 protocol
overview
Add locks on both the temperature and magnetic modules to prevent multi-access to data from the hardware.
review requests
Test Plan