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

Refactor platform programming API to better support multiple programmers #428

Closed
FFY00 opened this issue Jul 9, 2020 · 16 comments
Closed

Refactor platform programming API to better support multiple programmers #428

FFY00 opened this issue Jul 9, 2020 · 16 comments
Labels

Comments

@FFY00
Copy link
Contributor

@FFY00 FFY00 commented Jul 9, 2020

Right now each platform must define a toolchain_program function which will know how to program the device. The issue is there are tons of ways of programming a device. I propose that we teach nmigen how to interact with openocd, dfu-util, openFPGAloader, etc. and then have the platforms register their custom information for each of the programming interfaces they want to support.

Long-term I am also seeing the possibility of people being able to create plugins to extend the programming interfaces. This would make so that with nmigen being more widely adopted, it did not fall on the upstream to maintain all possible interfaces. It should be fairly straight-forward, but not that relevant right now.

Example taken from fomu_hacker.py:

def toolchain_program(self, products, name):
    dfu_util = os.environ.get("DFU_UTIL", "dfu-util")
    with products.extract("{}.bin".format(name)) as bitstream_filename:
        subprocess.check_call([dfu_util, "-D", bitstream_filename])

I am open to contribute this change. Let me know if it makes sense right now, if you want to wait, if there are other things blocking it, etc. 😁.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

The issue is there are tons of ways of programming a device.

Like where? Every board we support so far has a single primary way of programming it, so a single function is sufficient.

@cr1901
Copy link
Contributor

@cr1901 cr1901 commented Jul 9, 2020

Arty A7 can either use xc3sprog or openocd to program the flash. I've only have experience w/ the latter, but nmigen hardcodes the former.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 9, 2020

Right, I guess that's one of the few exceptions.


In general, I'm not a fan of deduplicating code for the sake of it. Suppose we factored out some sort of interface through which we run OpenOCD. Now two things happen:

  • Any change to the shared code needs to be tested on all the boards it affects. Too bad, since you likely don't have the board, the toolchain, or any easy contact with the person who contributed it.
  • Any special case requires either changing the shared code (see above), or duplicating it.

So there needs to be a more compelling reason to factor out programmer interfaces than "it saves around 5 lines of clear self-contained code per board on average". Right now, the public interface is the toolchain_program function, and in my view that's highly advantageous--any changes are localized to that specific board, and a board can do things that are as weird as it needs them to be.

Note that this contrasts with our various Resources, which are factored out because we expect various cores (from nmigen-stdio as well as external ones) to rely on the resource layout. In other words, the public interface is the full structure of the resource, and any changes to this structure can (and must) be manually verified in some way with every single board.

@FFY00
Copy link
Contributor Author

@FFY00 FFY00 commented Jul 10, 2020

Yes, and the current approach works perfectly fine for that. But I think it is also common for people to program via JTAG or something similar, am I wrong? This is where the current approach fails. When it comes to JTAG or another standard interface, there are multiple ways to do it.
For the boards that have an onboard way to program the FPGA, I'd also imagine that people sometimes might want/need to program via JTAG.

This approach would also benefit the current boards with onboard programmers, since you could unify and standardize the interface in one place. You wouldn't need to keep copying the same code between each platform declaration. It also allows you to standardize the interface. In the example above, you can pass DFU_UTIL as an env var, some other platform might do the same but call it DFUUTIL.

@FFY00
Copy link
Contributor Author

@FFY00 FFY00 commented Jul 10, 2020

Any change to the shared code needs to be tested on all the boards it affects. Too bad, since you likely don't have the board, the toolchain, or any easy contact with the person who contributed it.

I think this can be implemented without breaking the current code. We can keep toolchain_program as one of the backends and make it the default action. People can then start replacing toolchain_program usages with the openocd definitions, for eg., on their own.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

When it comes to JTAG or another standard interface, there are multiple ways to do it.

This isn't actually specific to JTAG. There's (quite literally) an infinite amount of ways in which you might want to program a board. And nMigen already supports them all! What you do is you use the (public) interface platform.extract to grab the bitstream and then program it in whatever way you want. This is similar to how nMigen already supports virtually every FPGA toolchain, since you can always ask it to just produce Verilog for you.

It is not a goal of nMigen or nmigen-boards to support, even in theory, every conceivable way to program a board. The goal is to provide a smooth out of the box experience for people who are just using their board in the primary way the vendor suggests it to be used. Anything more? You have a perfectly good Python interpreter right there.

You wouldn't need to keep copying the same code between each platform declaration.

I consider the current approach an advantage. Take a look at this essay for a more in-depth exploration of my view here.

It also allows you to standardize the interface. In the example above, you can pass DFU_UTIL as an env var, some other platform might do the same but call it DFUUTIL.

That doesn't require building a new abstraction. We can add something like nMigen's _toolchain.require_tool to ensure environment variable names match tool names. I haven't used it as-is because _toolchain is a private API and because there would be potentially undesirable interaction with how NixOS packages nMigen.

People can then start replacing toolchain_program usages with the openocd definitions, for eg., on their own.

That doesn't require building a new abstraction. People can do this just fine right now by doing the same thing toolchain_program does (or inheriting from the Platform they use and replacing that function, or replacing it on the instance, or whatever).

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

But I think it is also common for people to program via JTAG or something similar, am I wrong?

I wouldn't say it's all that common. I believe most of the time most people use the built-in programmer (which usually uses JTAG, internally routed on the board, at least on the FPGAs that do have JTAG), since that's the most convenient approach. For the rest? Figuring out the right options to pass to the nMigen abstraction would probably take more time (and be more frustrating) than just invoking subprocess.check_call themselves.

@cr1901
Copy link
Contributor

@cr1901 cr1901 commented Jul 10, 2020

The goal is to provide a smooth out of the box experience for people who are just using their board in the primary way the vendor suggests it to be used.

Sometimes there isn't a vendor-suggested (command-line) primary way. If we use arty and mercury as examples (see below), a programmer is added to a board based on "who implements toolchain_program and files a PR first?". Effectively, this means that built-in programmer support in nmigen-boards will be tailored first-come, first-serve to the use case of the PR submitter. Which might be fine for you! But if there isn't a single primary way to program a board (which yes, is a value judgment), I think I'd rather nmigen not support toolchain_program favor any particular programmer app/cable over another.

E.g. Digilent suggests you use Vivado for Arty (is there a command line programmer for Arty?). I have Mercury use mercpcl because there wasn't any command-line programmer before I wrote one. But the vendor suggests using a GUI C# application.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

Effectively, this means that built-in programmer support in nmigen-boards will be tailored first-come, first-serve to the use case of the PR submitter.

I didn't mean to say that nmigen-boards will support one and only one way to program every board. (I was actually wondering if I should clarify that in my earlier comment.) Ultimately, what is supported there is determined by the convenience of a typical nMigen user encountering that board. If it's approximately about as likely that such a typical user would want to use xc3sprog as openocd, then we can support both! There could be a default, or the default could be to throw an exception.

E.g. Digilent suggests you use Vivado for Arty (is there a command line programmer for Arty?).

You can use iMPACT via CLI. Atlys does this. If we do support it for Arty, then iMPACT should probably be the default.

I have Mercury use mercpcl because there wasn't any command-line programmer before I wrote one. But the vendor suggests using a GUI C# application.

This is an interesting edge case. Because, clearly, nmigen-boards can only run a CLI application (or perhaps a batch mode GUI application, though this is already marginal), your programmer seems to be the only one that fits its restrictions. So we can use it in toolchain_program just fine.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

But if there isn't a single primary way to program a board (which yes, is a value judgment), I think I'd rather nmigen not support toolchain_program favor any particular programmer app/cable over another.

All that said, if there really is nothing that a typical nMigen user is likely to find usable, then of course we should leave it out. For example, SK_XC6SLX9Platform only exposes a JTAG header. I didn't put a Glasgow invocation into toolchain_program because, although obviously usable for some, this isn't usable for a typical user.

@FFY00
Copy link
Contributor Author

@FFY00 FFY00 commented Jul 10, 2020

I want to go back to the original issue and better explain my motivations.

There are two issues. The main one is that there are several possible ways to configure a board, and nmigen only supports defining one. The other one is that the interface to pass arguments to the programmer is not standardized.

I was not clear about this but the duplicated code issue was never about code optimization, it was about the interface, which diverges between boards.

To me, having a standard interface for openocd for eg. would be a great step forward. I can use openocd with a wide range of programmers, I need to tell openocd how to do it.

My proposal to abstract the programmer was so that we can standardize the interface. We could have openocd add an argument to the build/CLI to receive my config(s).

Anyway, it is ultimately your call. I think the abstraction makes sense for standardizing the interface. Even if you decide against it, just being able to declare multiple programming interfaces would be a big big plus for me.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

which diverges between boards.

Where does it currently diverge between boards? That sounds like a bug.

@FFY00
Copy link
Contributor Author

@FFY00 FFY00 commented Jul 10, 2020

Sorry, bad wording. I mean in a design sense, there is no enforcement, it is completely defined by the platform.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

I can use openocd with a wide range of programmers, I need to tell openocd how to do it.

So I don't actually see how that would work. I looked at the openocd invocations currently in nmigen-boards. All of them are different, pretty significantly so. That's fine! Boards tend to be a bit (sometimes, very) unique, so having the openocd abstraction be subprocess.check_call([openocd, <board_specific_stuff>]) is totally fine. After all you are using a well-defined interface here--openocd's CLI.

I mean in a design sense, there is no enforcement, it is completely defined by the platform.

Right, so that's one benefit of this change. I think it does not outweigh the drawbacks, which is the need to test every board affected by changes in common code.


Going back to your earlier comment...

The main one is that there are several possible ways to configure a board, and nmigen only supports defining one.

We can just decide on what to do here. For example, we can decide that on every board with multiple ways to program it, toolchain_program takes a programmer="<tool>" keyword argument.

The other one is that the interface to pass arguments to the programmer is not standardized.

I very specifically do not want to provide a way to pass arguments in general. For example, I do not want to provide a way to customize the openocd script. toolchain_program should be configurable enough, and just enough, to cover the most typical use cases. (Changing the serial port, like in blackice, definitely counts as necessary configuration.)

If you want to customize things beyond that, use your own programming code.

@FFY00
Copy link
Contributor Author

@FFY00 FFY00 commented Jul 10, 2020

So I don't actually see how that would work. I looked at the openocd invocations currently in nmigen-boards. All of them are different, pretty significantly so. That's fine! Boards tend to be a bit (sometimes, very) unique, so having the openocd abstraction be subprocess.check_call([openocd, <board_specific_stuff>]) is totally fine. After all you are using a well-defined interface here--openocd's CLI.

I checked and most pass a config for the onboard programmer. We can make that the default config and allow the user to override it if they want to use a different programmer. We basically split the programmer and board specific stuff. The board specific definitions would always be included, but the programmer could be replaced.

Right, so that's one benefit of this change. I think it does not outweigh the drawbacks, which is the need to test every board affected by changes in common code.

Not really, because we don't need to move all devices to this new API. They can still keep their toolchain_program definition as is and eventually migrate. I don't want to break anything! It should be all backwards compatible.

I very specifically do not want to provide a way to pass arguments in general. For example, I do not want to provide a way to customize the openocd script. toolchain_program should be configurable enough, and just enough, to cover the most typical use cases. (Changing the serial port, like in blackice, definitely counts as necessary configuration.)

If you want to customize things beyond that, use your own programming code.

We can control that in the board/interface definitions we provide, so we can limit it to only necessary configurations. In this case, openocd would only allow you to provide your own config.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 10, 2020

I don't want to break anything! It should be all backwards compatible.

I don't really care about right now. As more time passes and more boards migrate to this common interface, changing this interface will become more and more hazardous and eventually it'll just be effectively frozen because we'll lose the ability to test every board affected by the change.

We can control that in the board/interface definitions we provide, so we can limit it to only necessary configurations. In this case, openocd would only allow you to provide your own config.

I don't see any reason to add an inherently leaky abstraction when the current approach works just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants