0.9 generates .json parameter that nextpnr can't parse due to extra <space> character (worked in 0.8)

[ret]

Steps to reproduce the issue

See gist files to help reproduce.

The issue is present in Yosys 0.9+1 (git sha1 a0b666f2, clang 10.0.1 -fPIC -Os). Worked in Yosys 0.8+ 615 (git sha1 a02d1720, clang 10.0.1 -fPIC -Os)

Expected behavior

No extra spaces in .json parameter (PULLUP), i.e. "1".

Actual behavior

Extra space "1 " in .json parameter (PULLUP), which nextpnr fails on.

Details

Yosys 0.9+1 (git sha1 a0b666f2, clang 10.0.1 -fPIC -Os) produces an extra space in "1 " for the PULLUP parameter below:

        "uart_0__rx_0": {
          ...
          "parameters": {
            "IO_STANDARD": "SB_LVTTL",
            "PIN_TYPE": 1,
            "PULLUP": "1 "
          },
	  ...

        "uart_0__tx_0": {
          ...
          "parameters": {
            "IO_STANDARD": "SB_LVTTL",
            "PIN_TYPE": 25,
            "PULLUP": "1 "
          },
          ...

This trips up nextpnr to error out with

ERROR: expected numeric value for parameter 'PULLUP' on cell 'uart_0__rx_0'

Version Yosys 0.8+ 615 (git sha1 a02d1720, clang 10.0.1 -fPIC -Os) did not produce the extra space:

        "uart_0__rx_0": {
          ...
          "parameters": {
            "IO_STANDARD": "SB_LVTTL",
            "PIN_TYPE": 1,
            "PULLUP": "1"
          },
	  ...

        "uart_0__tx_0": {
          ...
          "parameters": {
            "IO_STANDARD": "SB_LVTTL",
            "PIN_TYPE": 25,
            "PULLUP": "1"
          },
          ...

Reverting 15fae35 removes the problem (as a workaround).

I believe https://github.com/YosysHQ/yosys/blob/master/backends/json/json.cc#L102 may be the problematic line and will send a separate pull request to change it to add the space only if state == 1.

[daveshah1]

The space is deliberate to disambiguate between strings used to bit vectors and actual strings that contain only [01xz]*. I have implemented this on the nextpnr side in YosysHQ/nextpnr#306, can you confirm you are testing with nextpnr recent enough to include this? If so, then this is a nextpnr issue that should be fixed.

[daveshah1]

Incidentally, I don't think the value for pullup should be inside a string in the first place, the vendor examples show 1'b1 being used.

[ret]

can you confirm you are testing with nextpnr recent enough to include this? If so, then this is a nextpnr issue that should be fixed.

Ran with nextpnr at cce5cb65c108140d997eb60ffd92a9fc7ad02a5c (8/27).

[ret]

Incidentally, I don't think the value for pullup should be inside a string in the first place, the vendor examples show 1'b1 being used.

I'll try to see if I can use this moment to learn more about nMigen, but suspect that I'll need some help to address the 1'b1 as opposed to "1" generation ;-). Maybe I just change iCEBreaker board defintion for the UART PULLUPs (may be present in other board too).

[daveshah1]

Right, having looked into the code a bit more it isn't a bug at all. As per my second comment, PULLUP is supposed to be an actual number not a number inside a string (at least the vendor examples and Yosys cell library seem to use this), which is as good a reference as any).

This is no longer ambiguous (previously the JSON format didn't differentiate between numbers in strings and bit vectors in some cases, but this was a horrible bug) so nextpnr now catches it as an error.

[ret]

OK, Got it. I think I can change the PULLUP in https://github.com/m-labs/nmigen-boards/blob/master/nmigen_boards/icebreaker.py#L30. I'll open a note in nMigen and see if we can address this in the board definitions (more than iCEBreaker).
Thanks for your swift guidance, here!