Summary: | decoder doesn't show register writes unless LE goes low again after active low-to-high edge | ||
---|---|---|---|
Product: | libsigrokdecode | Reporter: | Martin Homuth-Rosemann <homuth-rosemann> |
Component: | PD: adf435x | Assignee: | Nobody <nobody> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | Gerhard.Sittig, vpalmu |
Priority: | Normal | ||
Version: | 0.5.3 | ||
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
capture: program adf435xctl register write
Testing the patched decoder Last frame View with the cleanup and frame error warnign patch applied |
Description
Martin Homuth-Rosemann
2022-12-06 17:36:48 CET
Created attachment 781 [details]
Testing the patched decoder
I see no difference in the output of the patched and unpatched decoders. Top is unpatched, bottom with the patch.
Created attachment 782 [details]
Last frame
Actually sorry, it does make a difference in the last frame sent.
It doesn't get decoded without the patch.
A cleaner patch:
19:06 $ diff pd.py ../adf435x/pd.py
132c132
< if data1 == 0:
---
> if data1 == 1:
data1 is the state before the transition, data2 the state after the transition.
Do you say that it's not the select line's going inactive, but instead it's the data length "becoming sufficiently long" while select is active, that makes the chip accept the data and latch it? If that'd be the case, then the decoder should reflect it. If the current implementation depends on the select line's changes, and doesn't see that change (because it's not part of the input data), then there's not much which the PD could do (without adding the risk of lying, which complicates matters even more). Got the suspicion that this is somehow related to the long lasting EOF discussion (see bug 292 and its duplicates). Or that the PD implements something different than what the chip's expected behaviour would be. Either cause needs to get addressed, after getting identified and verified as the cause of the difference that you observe between the decoder's behaviour and your expectation. The amount of changes or their size doesn't matter when the question is what's the problem and how to most appropriately fix it. Adding comments to those locations where non-trivial conditions are checked and how the implementation maps to real life's conditions sounds useful. The above discussion suggests that the implementation is easy to misread given the similarity of names, and names not reflecting the signal's meaning. Suggest to clean up this detail before addressing application issues. In this case there shouldn't be any mystical EOF stuff or other such things happening, it is just the current PD implementation triggering on the wrong edge; Per the datasheet the device shifts in data when LE is low and latches that into its registers on the rising LE edge. The current PD shifts in data when LE is low and decodes the data on a falling LE edge. This means that the PD is one LE transition behind the actual device as per the datasheet, and thus the last frame of the capture doesn't get decodes since there is no falling edge after it. Did a minor cleanup pass on the decoder in: https://github.com/depili/libsigrokdecode/commit/b6d8b29c73c950838f2c46ec868e29020f7daf15 This also adds a warning on frames with wrong number of shifted in bits. The warning makes it quite clear that the decoder state is wrong without also changing the trigger edge. https://github.com/depili/libsigrokdecode/commit/4e3794086ca2a99d81ecfff4ab4e21fef64e3c96 has the edge change and includes a comment in the code about the behavior of the device. Both commits are in the adf435x_fixes tree of my clone: https://github.com/depili/libsigrokdecode/tree/adf435x_fixes Created attachment 783 [details]
View with the cleanup and frame error warnign patch applied
This is a screenshot with just the first of the two patches from my tree being applied. Shows that the first decoding attempt happens on the first falling edge of LE and it will mark the whole start of the capture as being an invalid frame.
(In reply to vpalmu from comment #6) > This also adds a warning on frames with wrong number of shifted in bits. The > warning makes it quite clear that the decoder state is wrong without also > changing the trigger edge. Thx, looks nice - one remark, you write: https://github.com/depili/libsigrokdecode/blob/4e3794086ca2a99d81ecfff4ab4e21fef64e3c96/decoders/adf435x/pd.py#L153 [ANN_WARN, ['Frame error: Not enough bits', 'Frame error']] The error can also happen with too many bits, I changed it in my test setup to: [ANN_WARN, [f'Frame error: Wrong number of bits: {len(self.bits)}', 'Frame error']]) The patched decoder also passes the current test-case in sigrok-test without modifications. The only current test capture has short high spikes on the LE and thus doesn't trigger this issue. Would it be OK to include the capture attached in this bug as another testcase since it has different but still valid LE line usage? (In reply to vpalmu from comment #9) > The patched decoder also passes the current test-case in sigrok-test without > modifications. The only current test capture has short high spikes on the LE > and thus doesn't trigger this issue. > > Would it be OK to include the capture attached in this bug as another > testcase since it has different but still valid LE line usage? This is a good idea because the short spike capture is the correct timing according to the datasheet, while my attached capture uses the high-active latch enable LE more in the vein of a low-active /CE found on microcontrollers' integrated SPI. Finally had the time to massage this to something I think would be acceptable in https://github.com/sigrokproject/libsigrokdecode/pull/98 As a whole this moves the decoding logic to use the spi transfer events and thus relies on the CS polarity set in the spi decoder on the stack. The polarity needs to be set correctly in the first place to receive any data into the adf435x decoder, this just removes the hard coded one. Fixed in libsigrokdecode commit adb8233a0bf3. Thank you for reporting the issue, and for providing the more reliable approach to fix it. |