[doc] Error/warning on non-integer period on wave

[sjgitty]

.../docgen.py spi_device.md > spi_device.html ERROR: Clock must have an integer period

Seems to work though. Perhaps WARN or perhaps a real error lurking?

[sjgitty]

I researched this a bit. I didn't go deep enough into the wavesvg.py code but it appears to be visually doing the right thing by averaging down on the clock where it needs to. The offending line in the source wavedrom is this within spi_device.md

{ signal: [
  { name: 'SCK', wave: '0...p.|.|...|l' , node:'.............a', period:0.5},
}

Other uses of period:0.5 are fine, but because of the 'p' on the line, it is requesting a clock. The clock is rendered as if period 1, but the rest of the labeling needs to be 0.5 to align with the remainder of the waveform.

I recommend we change this to a warning or drop. But perhaps someone should dig deeper. Wavedrom is WYSIWYG so if it looks good, don't change it. A warning could alert you to being outside of range though.

[aytong]

Downgrade to P3. @sjgitty can you assign someone to update docgen.md to make this a WARNING, instead of ERROR, since the generated waveform looks ok? Thanks.

[sjgitty]

Well, this might just get raised again. Hugo is pointing to the official version of wavedrom so the 0.5 support might vanish all together. Let's leave and see if @gkelly has any thoughts in post-hugo landscape.

[towoe]

I had a look into this and I'm not completely sure I get it. According to wavedrom schema, period can only be a natural number, and the clock indicator should be used only at the beginning of the string. So my guess is we ignored those "rules" because it does work and the images look good? And the reason for this was to have the extended cycle with the gap, which resulted in two cycles? An alternative could be to use the barely documented sub-cycle feature https://github.com/wavedrom/wavedrom/issues/243 but I think it would still require some upstream work in order to remove some artifacts which are the result of not having the clock indicator at the first position and some sub-cycle resetting.

Current:

Modified:

diff --git a/hw/ip/spi_device/doc/_index.md b/hw/ip/spi_device/doc/_index.md
index 25557ed6..74e9ae49 100644
--- a/hw/ip/spi_device/doc/_index.md
+++ b/hw/ip/spi_device/doc/_index.md
@@ -87,18 +87,18 @@ edges, polarities, and bit orders are described later.

 {{< wavejson >}}
 { signal: [
-  { name: 'CSB',  wave: '10.........|....1.'},
-  { name: 'SCK',  wave: '0.p........|....l.'},
-  { name: 'MOSI', wave: 'z.=..=.=.=.=.=.=.=.=.=|=.=.=.=.z....',
+  { name: 'CSB',  wave: '10........<0|>0...1.'},
+  { name: 'SCK',  wave: '0n........<n|>n...l.'},
+  { name: 'MOSI', wave: 'z=========<=|>====z.',
     data:['R07','R06','R05','R04','R03','R02','R01','R00','R17',
-          '','R73','R72','R71','R70'], period:0.5, },
-  { name: 'MISO', wave: 'z.=..=.=.=.=.=.=.=.=.=|=.=.=.=.z....',
+          '','R73','R72','R71','R70'], },
+  { name: 'MISO', wave: 'z=========<=|>====z.',
     data:['T07','T06','T05','T04','T03','T02','T01','T00','T17',
-          '','T73','T72','T71','T70'], period:0.5}],
+          '','T73','T72','T71','T70'], }],
   head:{
     text: 'Data Transfer',
-    tick: ['-2 -1 0 1 2 3 4 5 6 7 8 9 60 61 62 63     ']
-  }
+    tock: ['-1 0 1 2 3 4 5 6 7 8  60 61 62 63   ']
+    }
 }
 {{< /wavejson >}}

[sjgitty]

@towoe yes you have the right way of looking at it. We used the half period because it happened to work, but gave the error in these few cases. I don't know if we care enough tbh to fix the source, but your proposed change looks "good enough". You could even remove the < and > for CSB to get rid of the unsightly "glitch", though it makes the S slightly misaligned.