wallento / wavedrompy

WaveDrom compatible python command line
Other
97 stars 23 forks source link

Issue with period=0.5 #8

Closed bavovanachte closed 5 years ago

bavovanachte commented 5 years ago

Given the following json

{
    "signal": [
            {"name": "sig", "wave": "x=...=...................=.....", "data": ["clock", "byte 0", "byte 1"]},
            {"name": "bit", "wave": "================", "data": ["","","","start",0,1,2,3,4,5,6,7,"stop", "start",0,1], "period": 2, "phase": 1},
            {"name": "bus", "wave": "h.nh.nh.l..hlx.hlx.hlx.hlx.hlx.hlx.hlx.hlx.hnh.l..hlx.hlx.h", "period": 0.5},
            {"name": "Data Link Layer", "wave": "0..............................................50.............", "period": 0.5}
        ],
        "foot": {
            "tock": 0
        },
        "config": { "hscale": 1 }
}

wavedrom produces this result: wavedrom

But wavedrompy produces this: wavedrompy

Could this have something to do with the fact that period is a decimal number?

wallento commented 5 years ago

Thanks for reporting. It is maybe something around that. Beside the fact that it should be fixed to match wavedrom behavior, I suggest you use subcycles for this case, which is probably better suited.

wallento commented 5 years ago

This will take a bit longer as it is a weird cornercase. You are right about the integer. There is only one non-integer case allowed and this is 0.5 I think. So I have tested adding some subsampling and it looks halfway okay, but then the use of "n" introduces a lot of weirdness in the code. wavedrom does not seem to apply the period to this one (compare vs. using "l" instead in the bus signal everywhere you use "n"). It seems solvable, but is a bit ugly.

wallento commented 5 years ago

That would be the wave with l instead of n:

{"name": "bus", "wave": "h.nh.nh.l..hlx.hlx.hlx.hlx.hlx.hlx.hlx.hlx.hnh.l..hlx.hlx.h", "period": 0.5},
wallento commented 5 years ago

That's the same with subcycles instead of period (which is cleaner IMHO):

{"name": "bus", "wave": "hnhnhln<lx.h><lx.h><lx.h><lx.h><lx.h><lx.h><lx.h><lx.h>nhln<lx.h><lx.h>"},
wallento commented 5 years ago

Solved in c5e792a

Your original code and my suggestions should all work now. I have also added regresssions to not break them in future. If you have time it would be awesome if you could add some of the more fiddly waveforms you are rendering as your own regressions. They are simple to add, you just need to generate the expected list of bricks like this:

w = WaveDrom()
w.lane.phase = 0
print(w.parse_wave_lane("p...", -0.5))

The second parameter is the "stretch", which calculates as hscale*period - 1. This would be highly appreciated!

bavovanachte commented 5 years ago

Thanks a lot for looking into this so fast! My colleague who found the issue checked your fix and it seems to work fine! I'll try to add the diagrams to the regression tests this weekend!

wallento commented 5 years ago

Awesome, thanks a lot. You can also send me wave descriptions that are good and I can create the regression if its easier.