Bug 1814

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: adf435xAssignee: 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 780 [details]
capture: program adf435xctl register write

When looking at a group of register writes, the last register is not decoded if the LE (Latch Enable, active positive edge) remains high. 
Used program: adf435xctl from project https://github.com/jhol/pyadf435x
System: Linux Debian Stable (bullseye)

See attached recording ADF435x_capture.sr

SPI setting: CLK = CLK, MOSI = DATA, #CS = LE

This is because the decoding function is only active on a CS-CHANGE from high to low.
This is contrary to what is stated in the ADF4351 data sheet:
"Load Enable. When LE goes high, the data stored in the 32-bit shift register is loaded into the register that is selected by the three control bits."

decoders/adf435x/pd.py

    ...
    def decode(self, ss, es, data):

        ptype, data1, data2 = data

        if ptype == 'CS-CHANGE':
            if data1 == 1:
                if len(self.bits) == 32:
    ...

My fix is to activate the decoder on a low to high CS-CHANGE:
            ...
            if data2 == 1:
            ...


--- pd.py.orig  2020-11-19 09:23:25.000000000 +0100
+++ pd.py       2022-12-06 16:13:28.027824790 +0100
@@ -129,7 +129,7 @@
         ptype, data1, data2 = data
 
         if ptype == 'CS-CHANGE':
-            if data1 == 1:
+            if data2 == 1: # HORO: fix
                 if len(self.bits) == 32:
                     reg_value, reg_pos = self.decode_bits(0, 3)
                     self.put(reg_pos[0], reg_pos[1], self.out_ann, [ANN_REG,
Comment 1 vpalmu 2022-12-06 17:57:58 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.
Comment 2 vpalmu 2022-12-06 18:00:36 CET
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.
Comment 3 vpalmu 2022-12-06 18:09:29 CET
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.
Comment 4 Gerhard Sittig 2022-12-07 08:31:49 CET
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.
Comment 5 vpalmu 2022-12-07 10:26:34 CET
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.
Comment 6 vpalmu 2022-12-07 11:49:11 CET
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
Comment 7 vpalmu 2022-12-07 11:51:06 CET
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.
Comment 8 Martin Homuth-Rosemann 2022-12-08 13:12:32 CET
(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']])
Comment 9 vpalmu 2022-12-08 13:21:32 CET
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?
Comment 10 Martin Homuth-Rosemann 2022-12-08 14:48:14 CET
(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.
Comment 11 vpalmu 2022-12-26 18:33:42 CET
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.
Comment 12 Gerhard Sittig 2023-01-09 20:19:21 CET
Fixed in libsigrokdecode commit adb8233a0bf3. Thank you for reporting the 
issue, and for providing the more reliable approach to fix it.