Events on hislip - #626
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
+ Coverage 48.19% 49.44% +1.24%
==========================================
Files 33 33
Lines 5625 5853 +228
Branches 545 577 +32
==========================================
+ Hits 2711 2894 +183
- Misses 2865 2891 +26
- Partials 49 68 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
oops, left some asserts in there. Will clean up |
|
Don't like all those asserts, but there were already a fair amount of them, so I'll leave them. |
|
will check any side effects of the latest merges |
|
@MatthieuDartiailh would you be OK with including the hislip locking handling (#635) in here? They are all to be handled in different parts of the code, so overview would still be possible. Would be easier for me, as it otherwise would mean sitting on 3 PRs that affect hislip. (and I have 2 more coming on VXI-11) |
|
TBH this one is already large so I would prefer you do not add more. You could try a stacked PR to let Github handle more of the boring resync. |
|
OK. Will place the HiSLIP locking in another PR, based on this one. |
|
OK to go. |
MatthieuDartiailh
left a comment
MatthieuDartiailh
left a comment
There was a problem hiding this comment.
Very partial review
| assert self.control_code == 0 | ||
| assert self.payload_length == 0 | ||
| self.vendor_id = struct.unpack("!4x4s8x", self.header) | ||
| self.vendor_id = struct.unpack("!4x4s8x", self.header)[0] |
There was a problem hiding this comment.
import struct
a = b"1234567890123456"
struct.unpack("!4x4s8x", a)
>>>>> (b'5678',)
struct.unpack("!4x4s8x", a)[0]
>>>>> b'5678'The last one we want (we want a simple string,). But when looking more at it, not sure if 4s is correct. Need to check. Later. Busy today.
|
|
||
| if "," in self.parsed.lan_device_name: | ||
| sub_address, port_str = self.parsed.lan_device_name.split(",") | ||
| parsed = cast(rname.TCPIPInstr, self.parsed) |
There was a problem hiding this comment.
This cast should not be needed since the rname is defined with the proper type on the class
| event_callback: Optional[Callable[[int], None]] = None, | ||
| interrupt_callback: Optional[Callable[[int], None]] = None, |
There was a problem hiding this comment.
Couldn't the type of the argument be narrower (IntEnum) ?
There was a problem hiding this comment.
no, message.control_code and message.message_parameter are not enums.
| self._thread = threading.Thread(target=self._run, daemon=True) | ||
|
|
||
| def start(self) -> None: | ||
| if not self._thread.is_alive(): |
There was a problem hiding this comment.
This looks dangerous, you cannot restart a thread.
There was a problem hiding this comment.
That start is only called on init (line 722), just after the creation of the thread.
for more information, see https://pre-commit.ci
2 participants
black . && isort -c . && flake8with no errors. -> Is this still valid? It seems outdated.