Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raise error on space in fsm name #595

Closed
wants to merge 1 commit into from

Conversation

hellow554
Copy link

@hellow554 hellow554 commented Feb 11, 2021

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
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

whitequark commented Feb 11, 2021

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

@hellow554
Copy link
Author

hellow554 commented Feb 12, 2021

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
Copy link
Collaborator

awygle commented Feb 12, 2021

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
Copy link
Collaborator

awygle commented Feb 12, 2021

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
Copy link
Contributor

programmerjake commented Feb 12, 2021

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

@awygle
Copy link
Collaborator

awygle commented Feb 12, 2021

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
Copy link
Author

hellow554 commented Feb 12, 2021

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
Copy link
Author

hellow554 commented Feb 13, 2021

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 whitequark self-assigned this Dec 11, 2021
@whitequark whitequark added this to the 0.3 milestone Dec 11, 2021
@whitequark
Copy link
Member

whitequark commented Dec 11, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants