Device db generator #1
Conversation
|
Thanks @AUTProgram! |
hartytp
left a comment
hartytp
left a comment
There was a problem hiding this comment.
Good job @AUTProgram! Overall, that's a really good start and if I never have to manually type out a device db again it's too soon.
A mixture of stylistic/structural comments there, but nothing too major.
Have you run the code and checked it works?
I would separate this into two PRs: one for the infrastructure, and one that adds a version of Kasli that uses the infrastructure.
| @@ -0,0 +1,139 @@ | |||
| #Read devices_config from file generated when bitstream was built | |||
There was a problem hiding this comment.
For this kind of thing, I'd use a docstring rather than a comment. That way, users can find the documentation for a function func using help(func) and it can be automatically included into the proper documentation. PEP 8 has some advice on docstrings, but you can also see how they are used elsewhere in artiq/migen/misoc. NB any non-trivial arguments/return values should be documented using param: ... etc.
On the other hand, if the documentation is too trivial to be in a docstring then it's probably not worth the comment either!
| def devices_config_to_file(devices_config, filename): | ||
| with open(filename, "w") as f: | ||
| f.write("devices_config = ") | ||
| json.dump(devices_config, f, indent=4) No newline at end of file |
There was a problem hiding this comment.
Missing new line at end of file (PEP 8)
|
|
||
| self.add_rtio(rtio_channels) | ||
|
|
||
| cleanup_devices_config(devices_config) |
There was a problem hiding this comment.
Do we need this function? It might be clearer/nicer to have devices_config_to_file filter out non-serialisable things like phys when saving, rather than calling a cleanup function first.
There was a problem hiding this comment.
I guess that depends on whether in the future there might ever be other functions that use devices_config, but have a problem with non-serialisable entries. If those functions are called before devices_config_to_file, they would also need extra code for filtering. I guess right now it would be more concise to have it all in devices_config_to_file, so I'll merge the two.
|
|
||
| #Create PHYs and rtio channels for a dio device and return dictionary with information | ||
| #about those channels | ||
| def eem_dio(cls, eem, name): |
There was a problem hiding this comment.
as these functions are already in a module named eems do they need to begin with eem_? i.e. depending on how the user decides to import them, they can already be called using eems.dio etc.
There was a problem hiding this comment.
There are already functions called _dio, _urukul etc in the files provided for the kasli target, so I wanted to distinguish my functions from those
| @@ -0,0 +1,44 @@ | |||
| from artiq.gateware.targets.kasli import (_MasterBase, _SatelliteBase, _dio, | |||
|
|
||
| core_addr = "core_addr" #IP address of host | ||
|
|
||
| core_device = { |
There was a problem hiding this comment.
This is all standard boilerplate. I'd be tempted to add a function to generate it, like core_device(addr, has_DMA=True, ...)
| } | ||
| } | ||
|
|
||
| #Provide information for each device and channel. The keys of peripherals |
There was a problem hiding this comment.
This text should go in the doctoring for create_device_db
| if device == device_id: | ||
| info = custom_info | ||
| break | ||
| else: |
There was a problem hiding this comment.
Does this code work (I haven't tried it myself yet)? Maybe I'm being daft, but what is that else in response to? The whitespace looks off...
There was a problem hiding this comment.
Okay, scratch that. I didn't know python had that feature...
| name: { | ||
| "type": "local", | ||
| "module": "artiq.coredevice.ad9912", | ||
| "class": "AD9912", |
There was a problem hiding this comment.
We need a parameter to allow us to choose either the AD9910 or AD9912 versions of Urukul (there are two hw revisions with different DDS chips on).
| "module": "artiq.coredevice.ad9912", | ||
| "class": "AD9912", | ||
| "arguments": { | ||
| "pll_n": 10, |
There was a problem hiding this comment.
The ref clock frequency and pll multiplication factor need to be selectable via parameters (we will use 125MHz, but other people will use 100MHz).
| from artiq.build_soc import build_artiq_soc | ||
| from artiq import __version__ as artiq_version | ||
|
|
||
| from artiq.gateware.targets.kasli import (_dio, _urukul) |
There was a problem hiding this comment.
Those won't exist in the future. Don't rely on that. That's also the reason they have underscores. They should be refactored and become part of this endeavor to generate RTIO PHYs and the device_db.
|
|
||
| import json | ||
|
|
||
| #Create PHYs and rtio channels for a dio device and return dictionary with information |
There was a problem hiding this comment.
You will want to split this so that the definition of the RTIO channels and the device_db tooling is not in a single file for all EEMs but split.
| #Create PHYs and rtio channels for an urukul device and return dictionary with information | ||
| #about those channels | ||
| def eem_urukul(cls, eem, eem_aux, name): | ||
| info = { |
There was a problem hiding this comment.
It might be better to push all this info down into the PHYs themselves and introduce other classes that maintain the rest of the metadata that spans multiple PHYs.
If you really need thi dict-of-dict-and-lists, then at least document it.
3 participants
I have added the files for automatic generation of the device database.
Run "python kasli_test.py" with a valid device_db to generate a device_db_check file that explicitly contains the device_db dictionary.
eems.py and device_db_generation.py contain the functions necessary for creation of the devices_config dictionary (information about what was actually built on target) and the device_db dictionary.
This is a basic version, extra functionality can be added via additional arguments.