Improved PS/2 decoder, added stacked mouse decoder #104
Conversation
|
General notes :
|
|
Also, unit tests : yes, absolutely want those. You may want to add some captures in the sigrok-dumps repo; then add your tests in the sigrok-test repo. I suggest looking at the 'spiflash' tests for an example of a stacked decoder test, it should be fairly straightforward to apply to kbd/mouse tests. |
Thanks for the advice. I'll do some code cleanup and then squash the commits with better names. |
Detects if host or device is initiaing communication Outputs byte for stacked decoder Outputs human-readable "binary" for logging
Mouse decoder interprets the 3-byte packets for movement and clicking, but ignores the configuration commands from the host. Keyboard decoder interprets press and release messages for each key but does not keep track of state (capslock, shift, etc).
| @@ -18,9 +19,11 @@ | |||
| ## | |||
|
|
|||
| ''' | |||
| This protocol decoder can decode PS/2 device -> host communication. | |||
| This protocol decoder can decode PS/2 device -> host communication \ | |||
There was a problem hiding this comment.
just noticed - are you sure you need the trailing \ ? see e.g. the uart PD
There was a problem hiding this comment.
Yeah. I don't actually want to force a line break there, but I also don't want to exceed the standard column width (80 characters)
Improved name consistency to assist in unit tests "mouse" and "keyboard" decoders renamed to "ps2_mouse" and "ps2_keyboard" "ps2_packet" output renamed to simply "ps2"
- Changed start-bit to begin on data edge rather than clock edge - Increased timeout to accept borderline slow clock speeds - Changed Host RTS to look for the clock line held low for 100us, rather than a low clock on the falling edge of the data. Some hosts release the clock immidiately after pulling the data low, which may look simultaneous even at 1MHz sample rates. - Added Host RTS annotation - Removed confusing "annotation offset" constant, expanded Ann class with host annotations - Changed to not annotate bits if less than 9 are captured (incomplete word)
Host RTS is cleared if data is pulled low within 100us Start bit doesn't have a maxium length requirement, the timer now begins when the client pulls the clock low (after the host has released it)
|
Man. I keep thinking I'm done and then I find more edge-cases. |
|
This is due to the minimum clock rate of 10kHz, so one bit period becomes
100 microseconds.
Would it be more pythonic to enter the constant in the constructor, and to
use Hz instead of seconds?
…On Sat, Apr 29, 2023, 7:09 AM fenugrec ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In decoders/ps2/pd.py
<#104 (comment)>
:
> @@ -62,65 +81,87 @@ def reset(self):
def start(self):
self.out_ann = self.register(srd.OUTPUT_ANN)
-
- def putb(self, bit, ann_idx):
- b = self.bits[bit]
- self.put(b.ss, b.es, self.out_ann, [ann_idx, [str(b.val)]])
-
- def putx(self, bit, ann):
+ self.out_py = self.register(srd.OUTPUT_PYTHON)
+
+ def metadata(self,key,value):
+ if key == srd.SRD_CONF_SAMPLERATE:
+ self.samplerate = value
+
+ def get_bits(self, n, edge:'r', timeout=100e-6):
would suggest adding a note on why a hardcoded timeout value is needed
here, and why its value should be 100E-6
—
Reply to this email directly, view it on GitHub
<#104 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABKY7NF3JNXKBKFR77SBC3XDUOLHANCNFSM6AAAAAAW3ZABGQ>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Not sure. I'm always wary of unexplained magic constants in code; if they're not meant to be changed by the user at runtime then IMO it should at least be well documented in the code. |
Refactored timeout into a class property instead of a function argument. Now based on minimum clock frequency, rather than maximum clock period
PS/2 decoder now recognizes host communication
Mouse decoder recognizes standard streaming mouse messages and provides human-readable "binary" output
I haven't made unit tests yet, I was hoping for some pointers on that.