raise error on space in fsm name #595

hellow554 · 2021-02-11T21:49:51Z

This testcase will throw an error on gtkwave start:

from nmigen.sim import Delay, Simulator

class A(Elaboratable):
    def elaborate(self, _):
        m = Module()

        with m.FSM(name="space here"):
            with m.State(0):
                pass
        return m

if __name__ == "__main__":
    def proc():
        yield Delay(1e-4)

    m = Module()
    m.submodules.a = A()
    sim = Simulator(m)
    sim.add_process(proc)
    with sim.write_vcd("broken.vcd"):
        sim.run()

GTKWave Analyzer v3.3.108 (w)1999-2020 BSI

Near byte 172, $VAR parse error encountered with 'top.a.space'
No symbols in VCD file..nothing to do!

After this: nmigen will throw this error:

Traceback (most recent call last):
  File "/home/marcel/projects/fpgastuff/a.py", line 21, in <module>
    sim = Simulator(m)
  File "/home/marcel/projects/fpgastuff/.VE/lib/python3.9/site-packages/nmigen/sim/core.py", line 66, in __init__
    self._fragment = Fragment.get(fragment, platform=None).prepare()
  File "/home/marcel/projects/fpgastuff/.VE/lib/python3.9/site-packages/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/marcel/projects/fpgastuff/.VE/lib/python3.9/site-packages/nmigen/hdl/dsl.py", line 540, in elaborate
    fragment.add_subfragment(Fragment.get(self._named_submodules[name], platform), name)
  File "/home/marcel/projects/fpgastuff/.VE/lib/python3.9/site-packages/nmigen/hdl/ir.py", line 39, in get
    obj = obj.elaborate(platform)
  File "/home/marcel/projects/fpgastuff/a.py", line 10, in elaborate
    with m.FSM(name="space here"):
  File "/usr/lib/python3.9/contextlib.py", line 117, in __enter__
    return next(self.gen)
  File "/home/marcel/projects/fpgastuff/.VE/lib/python3.9/site-packages/nmigen/hdl/dsl.py", line 361, in FSM
    raise ValueError("FSM name '{}' contains space, which is not allowed".format(name))
ValueError: FSM name 'space here' contains space, which is not allowed

Other character, even non visible, e.g. \x03 seems fine, only a space \x20 is bad.

codecov · 2021-02-11T21:52:56Z

Codecov Report

Merging #595 (7e8f896) into master (f7c2b94) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
- Coverage   81.50%   81.47%   -0.03%     
==========================================
  Files          49       49              
  Lines        6461     6463       +2     
  Branches     1287     1288       +1     
==========================================
  Hits         5266     5266              
- Misses       1007     1009       +2     
  Partials      188      188

Impacted Files	Coverage Δ
nmigen/sim/pysim.py	`90.07% <0.00%> (-0.73%)`	⬇️
nmigen/build/run.py	`22.05% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7c2b94...7e8f896. Read the comment docs.

whitequark · 2021-02-11T21:56:19Z

Why not replace spaces with underscores instead? This is arguably a gtkwave bug.

hellow554 · 2021-02-12T16:08:14Z

Why not replace spaces with underscores instead

I think that would be a weird feature. So you couldn't use foo_a and foo a together, because they would clash and the programmer wouldn't know why.

This is arguably a gtkwave bug.

I haven't found a specification for vcd files, specifically for the id/name. Can you help out here?

awygle · 2021-02-12T17:53:51Z

I haven't found a specification for vcd files, specifically for the id/name. Can you help out here?

The Extended VCD File Format is defined in IEEE 1364-2001 and later revisions, however it doesn't shed much light on this issue.

I did find this GTKWave thread about escaping spaces as hex or octal, but the context isn't quite clear. I'd suggest you try that and see what results you get.

EDIT: OK I tried it and it doesn't work, also GTKWave gets somewhat cranky about names with backslashes in them (dropping everything before the backslash in the signal view).

awygle · 2021-02-12T18:11:51Z

Is it possible to migrate this error to the VCD writer? I'm not sure I agree with whitequark that this is a GTKWave bug exactly, I think it's actually a deficiency in the VCD file format. Throwing from the VCD writer would prevent creating useless VCD files without restricting the names that can be used in nmigen proper.

programmerjake · 2021-02-12T18:15:17Z

I think it would be better to escape spaces when writing vcd files, rather than failing

awygle · 2021-02-12T18:24:01Z

I think it would be better to escape spaces when writing vcd files, rather than failing

I agree but I don't think there's an unambiguous way to do so. In the "foo a" vs "foo_a" example above, what do you do?

hellow554 · 2021-02-12T23:43:08Z

I don't like it when the program does something that I didn't intend to do.

As @awygle and me already saying, you are creating two different names but both will have the same name in the end, because the program does something not very obvious.
Then the next person will spent an hour to figure out what the heck is going on and files the next bug. So raising an error is the lesser evil, especially when it's clear what to do (e.g. replace the space).

I'm more than willing to raise the error in the vcdwriter. Will have (hopefully) time tomorrow to do that.

hellow554 · 2021-02-13T23:14:29Z

The ValueError will now be raised in the VCDWriter instead, so it catches all variables that contain a space.

I think this is the better solution :)

whitequark · 2021-12-11T11:13:24Z

Thanks for the report and the PR! I've implemented a slightly extended version of the check in 66295fa that covers the grammar recognized by GTKWave, and added a test.

hellow554 force-pushed the master branch from ae0cc80 to 245f527 Compare February 13, 2021 23:12

raise valueerror on space in variable name in vcdwriter

7e8f896

hellow554 force-pushed the master branch from 245f527 to 7e8f896 Compare February 13, 2021 23:13

whitequark self-assigned this Dec 11, 2021

whitequark added this to the 0.3 milestone Dec 11, 2021

whitequark closed this in 66295fa Dec 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raise error on space in fsm name #595

raise error on space in fsm name #595

hellow554 commented Feb 11, 2021

codecov bot commented Feb 11, 2021 •

edited

Loading

whitequark commented Feb 11, 2021

hellow554 commented Feb 12, 2021 •

edited

Loading

awygle commented Feb 12, 2021 •

edited

Loading

awygle commented Feb 12, 2021

programmerjake commented Feb 12, 2021

awygle commented Feb 12, 2021

hellow554 commented Feb 12, 2021 •

edited

Loading

hellow554 commented Feb 13, 2021

whitequark commented Dec 11, 2021

raise error on space in fsm name #595

raise error on space in fsm name #595

Conversation

hellow554 commented Feb 11, 2021

codecov bot commented Feb 11, 2021 • edited Loading

Codecov Report

whitequark commented Feb 11, 2021

hellow554 commented Feb 12, 2021 • edited Loading

awygle commented Feb 12, 2021 • edited Loading

awygle commented Feb 12, 2021

programmerjake commented Feb 12, 2021

awygle commented Feb 12, 2021

hellow554 commented Feb 12, 2021 • edited Loading

hellow554 commented Feb 13, 2021

whitequark commented Dec 11, 2021

codecov bot commented Feb 11, 2021 •

edited

Loading

hellow554 commented Feb 12, 2021 •

edited

Loading

awygle commented Feb 12, 2021 •

edited

Loading

hellow554 commented Feb 12, 2021 •

edited

Loading