wavedrom icon indicating copy to clipboard operation
wavedrom copied to clipboard

More sub-cycle tests

Open drom opened this issue 5 years ago • 6 comments

h<.>l

{signal: [
  {name: 'pulse', wave: '0.h<.>l.'}
]}

expected: 1.5 cycle active-high pulse

<h>.l

{signal: [
  {name: 'pulse', wave: '0.<h>.l.'}
]}

expected: 1.5 cycle active-high pulse

.<h..>l

{signal: [
  {name: 'pulse', wave: '0.<h..>l.'}
]}

expected: 1.5 cycle active-high pulse

drom avatar Jul 14 '19 04:07 drom

I have a few more as I worked a lot on fixing this in wavedrompy. They are from my regressions (https://github.com/wallento/wavedrompy/blob/master/test/regressions.py#L85)

<10>10

{ "signal": [
  { "name": "sig", "wave": "<10>10" }
  ]
}

wavedrom

Expected:

wavedrompy

<x0>1x

{ "signal": [
  { "name": "sig", "wave": "<x0>1x" }
  ]
}

wavedrom

Expected:

wavedrompy

<01>

{ "signal": [
  { "name": "sig", "wave": "<01>" }
  ]
}

wavedrom

Expected:

wavedrompy

x.<01...0>x

{ "signal": [
  { "name": "sig", "wave": "x.<01...0>x" }
  ]
}

wavedrom

Expected:

wavedrompy

<=|>.x

{ "signal": [
  { "name": "sig", "wave": "<=|>.x" }
  ]
}

wavedrom

Expected:

wavedrompy

x<=|>.x

{ "signal": [
  { "name": "sig", "wave": "x<=|>.x" }
  ]
}

wavedrom

Expected:

wavedrompy

x<.|>0x

{ "signal": [
  { "name": "sig", "wave": "x<.|>0x" }
  ]
}

wavedrom

Expected:

wavedrompy

wallento avatar Aug 29 '19 08:08 wallento

Aliaksei,

I have a patch that resolves all of the above rendering issues, as well as the signals from Cliff's e-mail report in Issue #163 / quirks in the closing remarks in the original enhancement request #148 .

The current rendering for the full test set is in this shortened wavedrom editor URL : https://tinyurl.com/y45gajkr ; Please rotate layout to compare with the rendering based on my patch - picture attached:

parse-wave-lane-ganeshts-patch-09102019-test-set

I am not really familiar with Git stuff yet, so I am just attaching the single file that I modified as part of my patch as a ZIP archive.

parse-wave-lane-ganeshts-patch-09102019.zip

One aspect that I could not fix from within that single file is the matching of the data strings to the multi-bit wave characters - It looks like the matching needs the multi-bit character to be at least 2 bricks wide - so, we don't get the data in multi-bit characters that are part of sub-cycles and rendered for only one brick.

Unfortunately, my build environment is a bit unconventional - My JavaScript experience is restricted to development of simple scripts called in a single HTML file and I use a lot of console log statements to debug. Apparently, the current WaveDrom build environment doesn't allow that. I had to build using the supplied instructions, copy over the generated wavedrom.js and wavedrom.min.js to a separate folder that had the editor.html from a download of the source code zip file, and test visually for the attached WaveJSON. I also didn't check for how nodes and annotations would match up with this sub-cycle feature (those don't seem to be part of the parse-wave-lane.js that I modified). Maybe I am missing some regression test that can be done to qualify the changes.

Please feel free to use / discard the patch, as you see fit.

Best Regards Ganesh

Ganesh-AT avatar Oct 09 '19 17:10 Ganesh-AT

Refactored the code attached in the ZIP above to reduce eslint complexity from 23 to 21

the single file that I modified as part of my patch as a ZIP archive.

parse-wave-lane-ganeshts-patch-09102019.zip

diff lib/parse-wave-lane.js lib/backed-up-parse-wave-lane.js
74c74,85
<                 if (!doGenFWB) {
---
>                 if (doGenFWB) {
>                     // Rendering a seamless transition or starting of the wave lane
>                     // for the desired number of repetitions (translated based on the period)
>                     if (numSegments != 0) {
>                         R = R.concat(genFirstWaveBrick(Next, 0, numSegments));
>                     }
>                     // Add a single brick render of the same type as the previously rendered ones
>                     // to handle the odd-boundary case
>                     if (halfCycleRender) {
>                         R = R.concat(genFirstWaveBrick(Next, 0, 0));
>                     }
>                 } else {
78,87c89,96
<                 }
<                 // Rendering a seamless transition or starting of the wave lane
<                 // for the desired number of repetitions (translated based on the period)
<                 if (numSegments != 0) {
<                     R = R.concat(genFirstWaveBrick(Next, 0, numSegments));
<                 }
<                 // Add a single brick render of the same type as the previously rendered ones
<                 // to handle the odd-boundary case
<                 if (halfCycleRender) {
<                     R = R.concat(genFirstWaveBrick(Next, 0, 0));
---
>                     // Handle the number of repetitions in the same manner as was done
>                     // for the seamless transition / wave lane starting case
>                     if (numSegments != 0) {
>                         R = R.concat(genFirstWaveBrick(Next, 0, numSegments));
>                     }
>                     if (halfCycleRender) {
>                         R = R.concat(genFirstWaveBrick(Next, 0, 0));
>                     }

Ganesh-AT avatar Oct 09 '19 19:10 Ganesh-AT

Aliaksei,

Can you shed more light on how this sub-cycle feature is supposed to behave with non-default hscale values?

For hscale = 2 and period = 1, should we be putting in 4 bricks per period? To be more exact, will we be supporting (as example) all of <2345> in a single period, or, should it be rendered as 2,3 in the first period and 4,5 in the next one?

Ganesh-AT avatar Oct 09 '19 20:10 Ganesh-AT

I have been thinking about this more, and IMO, users get the most flexibility if sub-cycle rendering is always in terms of the minimum possible brick size for the configuration. So, with hscale = 2, I think we should have each character inside a sub-cycle represent 1/4th of the period (in the example above, all of 2,3,4, and 5 would render within the space between two vertical markers). The patch above assumes hscale = 1, but, it is trivial to fix by adding the 'extra' parameter to the genWaveBrick call inside the subCycle-enabled block

if (!doGenFWB) {
    // Handling an abrupt transition from one type of brick to another
    // by first rendering the single brick dependent on both the old and current characters
    R = R.concat(genWaveBrick((Top + Next), extra, 0));
}

However, this breaks the rendering for WaveJSON strings with gap markers

  {name: "sig4", "wave": "<=|>.x" },
  {name: "sig5", "wave": "x<=|>.x" },
  {name: "sig6", "wave": "x<.|>0x" },

I am trying to figure out the code responsible for aligning the gap markers...

Ganesh-AT avatar Oct 10 '19 01:10 Ganesh-AT

The sub-cycle consideration in render-gaps.js was not differentiating between sub-cycle nodes and regular nodes when taking hscale into consideration. I am attaching a zip version of the two files (parse-wave-lane.js and render-gaps.js) that I have modified over the last couple of days. This version renders all test cases in the repository not related to the sub-cycle feature correctly (existing functionality is not broken AFAIK). All the test cases from various bug reports related to the sub-cycle feature are rendered in a well-defined manner for different hscale configurations and period values.

parse-wave-lane-render-gaps-patch-ganeshts-10092019.zip

@drom Please let me know if there is anything else I could help with in closing Issues #240 #243 and #244 - Documentation can be made ready quickly once we agree on how hscale will affect sub-cycle rendering (I already have some PDF slides assuming the behavior I have implemented)

Ganesh-AT avatar Oct 10 '19 06:10 Ganesh-AT