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

[RFC] handling boards that are very similar #89

Open
rroohhh opened this issue Jul 23, 2020 · 11 comments
Open

[RFC] handling boards that are very similar #89

rroohhh opened this issue Jul 23, 2020 · 11 comments
Labels

Comments

@rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Jul 23, 2020

There seem to be two subclasses of very similar boards:

  1. Boards that are completely equivalent wrt usable pins / connectors and resources and only differ in exact fpga model (usually size and / or speedgrade). These include the tinyfpga_ax{1,2} and the zturn_lite_z0{07s,10}.
    Currently these are handled as different boards, located in different files. The zturn_lite uses inheritance to avoid duplicating the pinout.
  2. Boards that are very close but still differ in resources (due to for example different sets of usable pins on different fpga models). These are currently also handled as completely different boards (for example the ecpix).

The approach of making every variant its own board currently seems to bring two main advantages:

  1. Its hard to use the wrong board (by accidentally using the wrong fpga model or speedgrade)
  2. Each variant is (apart from the zturn_lite) is completely seperate from each other, making maintance easier (changing a variant only ever affects that variant).

Considering these advantages the approach for the second class seems very reasonable already.

For the first class however it might be better to put them in the same file and use inheritance like the zturn_lite to avoid duplicating pinout, programming logic and more.

Especially for speedgrade however this still seems a bit unnecessary and could bloat the list boards.

@FFY00
Copy link

@FFY00 FFY00 commented Jul 24, 2020

This is related to the discussion in #75.

TL;DR
The DSL needs to be redesigned. @whitequark doesn't have time right now, I have offered to help, she will probably ping me when she has time.

Loading

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 25, 2020

I don't think this choice depends in any way on the redesigned DSL.

Loading

@whitequark whitequark mentioned this issue Jul 27, 2020
@Ravenslofty
Copy link

@Ravenslofty Ravenslofty commented Jul 27, 2020

What about the DE10-Nano and MiSTer situation? The latter is basically a board mounted on the former, and so pins which are free connectors on the DE10 are instead Resources for the MiSTer. Is that an instance of 2, or its own special case?

Loading

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 27, 2020

I'm not actually sure. I'd say special case, there wouldn't be that many of those.

Loading

@rroohhh
Copy link
Contributor Author

@rroohhh rroohhh commented Jul 27, 2020

@Ravenslofty good question, I think making the MiSTer inherit from the DE10-Nano and add its own Resources (like it is done here) would work.

Loading

@ktemkin
Copy link
Member

@ktemkin ktemkin commented Jul 27, 2020

Summarizing what I'm doing with the ULX3S, which theoretically has a variant for each FPGA type:

  • Creating a single abstract base class (lacking device as not to be instantiable) with the shared board definitions.
  • Creating a single subclass for each variant, located all in the same file.
  • Using argparse to select which variant should be used for the test code, like so:
if __name__ == "__main__":
    from .test.blinky import *
    
    variants = {
        '12F': ULX3S_12F_Platform,
        '25F': ULX3S_25F_Platform,
        '45F': ULX3S_45F_Platform,
        '85F': ULX3S_85F_Platform
    }
    
    # Figure out which FPGA variant we want to target...
    parser = argparse.ArgumentParser()
    parser.add_argument('variant', choices=variants.keys())
    args = parser.parse_args()

    # ... and run Blinky on it.
    platform = variants[args.variant]
    platform().build(Blinky(), do_program=True)

I'd summarize my suggestions as follows:

  1. For Platforms that are essentially variants of the same hardware, I'd consider keeping them in the same file, and using inheritance to deal with the deviations, as in my proposed 55facd6. This would mean the Versa and the Versa-5G would ideally be in a single file.
  2. For platforms that share resources, but are non-trivially different, I'd consider having multiple files, and using inheritance to share pieces iff there's a major gain from the non-repetition. I think a case-by-case basis works fine, here: if you think of a board as "adding something on" to another board, it might make sense to use inheritance, but I wouldn't go out of the way to remove redundancy.

One remaining question is whether "revisions" of a given board (e.g. OrangeCrab r0.1/r0.2) should be in separate files. I don't think this entirely matters, but I think that I'd default to treating them like variants (same file) if they're more or less the same hardware, and as separate platforms (different files) if the hardware's majorly changed. Example: If r0.2 board moves some pins around and changes an ADC, it'd be similar to a variant, and could hang around in the same file. If an r0.2 adds several new connectors, it'd be essentially a separate platform deserving of its own file.

Thoughts?

Loading

@rroohhh
Copy link
Contributor Author

@rroohhh rroohhh commented Jul 27, 2020

@ktemkin I like your suggestions, do you have a good idea on how to handle different speedgrades? I feel like adding a variant for each combination of speedgrade and size would bloat the number of boards quite alot, but that might be acceptable, as not that many boards seems to be available with different speedgrades.

Loading

@ktemkin
Copy link
Member

@ktemkin ktemkin commented Jul 28, 2020

I like your suggestions, do you have a good idea on how to handle different speedgrades

I'd consider treating them like any other variant (e.g. with FPGA size), and potentially changing our strategy only if we run into situations where we find boards with a wide variety of speed grades and sizes.

A couple of mitigating factors for that:

  • I haven't really seen that situation, yet.
  • With some families (e.g. the Lattice ECP5), there seems to be a good body of evidence that the speed grade number is effectively meaningless, and that Lattice doesn't seem to perform any actual binning.
  • This is python -- worst comes to worst, we can programmatically generate the combinations necessary. =P

Loading

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 28, 2020

I see several factors that can guide our decisions:

  • If there is an nmigen-boards class per every board that is actually sold (or otherwise shipped), that naturally limits the amount of classes we'll get, since it is usually disadvantageous for vendors to have lots of SKUs for no reason.
  • If downstream code is highly likely to need programmatic choice between board variants, then we should think about making those constructor arguments rather than separate board classes. But if a typical project would only care about one variant, then separate classes give a bit more clarity, since there are fewer concepts to keep in mind (different board = different class, no need to look if it has a constructor instead).
  • We can do both at the same time, so it's possible to go from one approach to another in a backward compatible way.

Loading

@awygle
Copy link
Contributor

@awygle awygle commented Jul 28, 2020

If downstream code is highly likely to need programmatic choice between board variants, then we should think about making those constructor arguments rather than separate board classes. But if a typical project would only care about one variant, then separate classes give a bit more clarity, since there are fewer concepts to keep in mind (different board = different class, no need to look if it has a constructor instead).

Most of my projects are developed on a dev board and on "real" hardware simultaneously, which means I need programmatic choice between boards, but not necessarily board variants, FWIW. If I do a new revision I'm likely to switch to it entirely rather than supporting old boards. But my projects are not, themselves, dev boards, so someone like Greg or Luke may feel differently.

Loading

@whitequark whitequark mentioned this issue Aug 5, 2020
@rroohhh
Copy link
Contributor Author

@rroohhh rroohhh commented Aug 5, 2020

I found another interesting case with the MicroZed (#99). Is is available in two variants (with Zynq 7010 and 7020) and those differ in which pins are available on the two connectors it has. (As the Zynq 7020 has one IO Bank more than the 7010). There are no other resources on it.

Sharing part of the connector definition between the two boards would be nice, however it is not quite clear how this should be done. The only way I can think of right now is putting the connector pins definition in a string template and then removing the pins not available using string processing in the 7010 variant, like this:

import re

connector_template =  "F9  J6  "
            "F6  G6  "
            "-   -   "
            "-   R11 "
            "R19 T19 "
            "T11 T12 "
            "T10 U12 "
            "-   -   "
            "B13_U13 B13_V12 "
            "B13_V13 B13_W13 "
            "-   -   "
            "T14 P14 "
            "T15 R14 "

class MicroZedZ010Platform(Xilinx7SeriesPlatform):
    ...

    connectors = [
        Connector("JX1", 0, re.sub(r'B13_[^\s]*', "- ", connector_template))
    ]

class MicroZedZ020Platform(Xilinx7SeriesPlatform):
    ....

    connectors = [
        Connector("JX1", 0, connector_template.replace("B13_", ""))
    ]

One downside of this approach is, that it is not as obvious as before (just writing the connector definition twice) what is happening.

Loading

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
6 participants