Add fast-servo linien gateware #37
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fsagbuya/nix-servo:gateware"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
firmware.patch is still an awful file name.
@ -34,0 +79,4 @@fast-servo-firmware = pkgs.python3Packages.buildPythonPackage {name = "fast-servo-firmware";src = pkgs.fetchFromGitHub {owner = "elhep";This is inconsistent. Either use elhep repositories everywhere or (preferred) use upstream linien with patches inside this repos.
This dependency is also outsourced in the patch here
f52548367e/install.sh (L9). However, the option to use elhep repo everywhere only use the linien version0.8.0and not the latest1.0.1. The currentlinien.patchhere is applied to version1.0.1. Which one should we employ?The main point of using Nix is not to have this kind of non-reproducible installation script. So whatever this script does regarding management of dependencies is not an example to follow.
As I said, better use upstream linien and apply the changes from elhep in the form of patches.
Understood. However running the gateware script of the patched linien from elhep would return an error such as this (since it depends on the other elhep repo):
We can build on top of the elhep patches to add the missing module.
Obviously you need to apply the elhep patch to the linien repos that it depends on.
@ -374,6 +456,7 @@patchShebangs qemu-script'';in {"fast-servo-gateware" = gateware;See all my previous comments about this kind of situation.
@ -34,0 +84,4 @@rev = "7fae40c0f872a91218be378f8289b98b1e366729";hash = "sha256-jqMF0fsicdRCq6kwJcZDcVRuHgskTpt7Qo4WkNkE028=";};# fix deprecated migen `add_source` errorIf you had named this file properly you would not need this comment.
The patch needs traceability information, e.g. "diff between linen-org/linien commit ID XXXXX and elhep/linien commit ID YYYYYY"
38090c9055to6cbe558c31@ -374,6 +445,7 @@patchShebangs qemu-script'';in {"${board}-gateware" = gateware;You need to disable this for zc706. There is no linien gateware for this board and probably will never be. Maybe just take the gateware out of board-package-set.
6cbe558c31tof1b6767e59@ -0,0 +1,4395 @@"diff between linen-org/linien commit ID e3b2dec and elhep/linien commit ID b73eea0"# https://github.com/elhep/linien/commit/b73eea07889dda8b55f0cf4c2afde96cf4c3efd1# Added the "fast_servo" module from https://github.com/elhep/Fast-Servo-FirmwareAgain, obviously the commit ID needs to be specified.
Putting both into the same patch sounds like maintainance hassle. And if you are just copying files then the patch format is not the right one.
May I know what do you mean by this?
Just put files into a directory and copy them.
@ -148,0 +204,4 @@vivado];buildPhase = ''export HOME=$(mktemp -d)What needs that?
It is a fix to prevent this error:
I've been using Nix for 6 years and I had already guessed that much.
@ -145,6 +190,32 @@};};gateware = pkgs.stdenv.mkDerivation rec {fast-servo-gateware = ...
f1b6767e59tof640be9b8f"gateware-support" is again a poorly chosen name. Should be "linien-gateware" or similar?
And why is there a pitaya_ps.py in the fast-servo gateware?
Looks like this could be better handled with a patch against the upstream Linien gateware, instead of all this copying.
There are some submodules defined in pitaya_ps that was used by the
BaseSOCmodule of fast-servo (i.e. Axi2Sys, Sys2CSR, SysCDC, SysInterconnect)Will revert back to the patch format, removing
pitaya_psand transferring the required modules inside the fast_servo_soc.py.Well that's silly but looks like an upstream issue which can be addressed later.
Just stick to upstream for now. Isn't the elhep stuff just minor modifications on top of the original linien gateware?
Well the elhep patch itself is 1200+ lines of codes already, not to mention that it imports the other module from
Fast-Servo-Gatewarerepo, which makes it longer.What "other module"? I don't find a Fast-Servo-Gateware repos. If you meant Fast-Servo-Firmware it seems to me that it's (1) stuff that we don't need and you can strip (2) minor modifications on top of upstream Linien. In this context 1200 lines counts as minor.
Yes, I meant the Fast-Servo-Firmware. I think the primary base board and peripheral definitions are inside the Fast-Servo-Firmware repos, also there are some python scripts there that needs to be run after boot up as per this instruction.
Loading the gateware with fpgautil is standard procedure for Zynq and outside the scope of this PR which should be focused solely on building the gateware.
Put the gateware init python scripts into a dedicated folder in this repos (with provenance information). Doesn't seem to be a big deal. But it should not be part of this PR either.
f640be9b8ftof19c3d09a4Again fast-servo-modules.patch is nonsense filename.
Seems those patches should be called linien-fast-servo-server.patch and linien-fast-servo-gateware.patch.
Why does the server patch have provenance information at its beginning but not the gateware one?
Will add provenance information as well.
@ -148,0 +200,4 @@name = "fast-servo-gateware";inherit (pkgs.python3Packages.linien-common) src;# Apply fast-servo board support# See: https://github.com/elhep/linien/commit/b73eea07889dda8b55f0cf4c2afde96cf4c3efd1To which patch does this commit refer to?
How does it relate to the provenance information in the patches?
Will clean this up and update to the match the provenance info from patches.
Removed the comment instead, since it will be described from the provenance info.
f19c3d09a4todc09533217@ -0,0 +1,2359 @@# Patch containing fast_servo gateware module from:# https://github.com/elhep/Fast-Servo-Firmware/tree/master/fast_servo/gatewareWhat can I say about that?
@ -0,0 +1,32 @@diff --git a/fast_servo/gateware/fast_servo_platform.py b/fast_servo/gateware/fast_servo_platform.pyindex 13b4aa3..89a8103 100644--- a/fast_servo/gateware/fast_servo_platform.py+++ b/fast_servo/gateware/fast_servo_platform.pyWhy is this not in linien-fast-servo-gateware.patch?
The patches still look like a complete mess. Please read again everything that has been said so far and pay attention.
dc09533217to68d0253a8dUpdates:
linien-gatewaredirectory, init python scripts excluded and add a README provenance info.Build log:
68d0253a8dtofe49ff7f57@ -0,0 +1,36 @@# Fix for migen add_source deprecation and removed xilinx bootgen command# .bin file is being generated by bit2bin.py from Linien repositoryWhy do we need the .bin?
It seems that is the format supported by the fpga manager when loading gateware inside Linux.
Okay then.
@ -148,0 +214,4 @@cp gateware/build/top.bit $outcp linien-server/linien_server/gateware.bin $outecho file binary-dist $out/top.bit >> $out/nix-support/hydra-build-productsecho file binary-dist $out/gateware.bin >> $out/nix-support/hydra-build-products(Assuming we need .bin at all)
@ -148,0 +214,4 @@cp gateware/build/top.bit $outcp linien-server/linien_server/gateware.bin $outecho file binary-dist $out/top.bit >> $out/nix-support/hydra-build-productsecho file binary-dist $out/gateware.bin >> $out/nix-support/hydra-build-productsWhy is it top.bit and gateware.bin? Why not top.bin?
Will rename it as top.bin.
#37 (comment)
Okay. Will retain it as is.
@ -148,0 +212,4 @@installPhase = ''mkdir -p $out $out/nix-supportcp gateware/build/top.bit $outcp linien-server/linien_server/gateware.bin $outThe directory name inconsistency in these two lines is very suspicious.
This is linien's default output directory for the bin file. I agree this is suspicious. Will add a patch to use the same directory for the top.bit.
If this is an upstream issue then fine. Just want to make sure it's understood and not introduced here.
fe49ff7f57tobcc12e1007@ -148,0 +193,4 @@fast-servo-gateware = pkgs.stdenv.mkDerivation rec {name = "fast-servo-gateware";inherit (pkgs.python3Packages.linien-common) src;postUnpack = ''Shouldn't this be done in prePatch, not postUnpack?
prePatch handles modification on
patchesattribute. It is selected here since it is only copying the files and not dealing with patches essentially.Where did you get that information from?
https://ryantm.github.io/nixpkgs/stdenv/stdenv/#var-stdenv-prePatch (and IIRC other sources) just say "Hook executed at the start of the patch phase."
Apologies, I maybe pointing to patchPhase not the prePatch. However applying it on prePatch returns a hunk error.
Should be fairly easy to debug with ls etc.
Running ls after
mkdir -p $sourceRoot/fast_servo/gatewareand it does not found the directory:As usual please figure out what exactly what is going on and sort it out.
Converted to
prePatch. It does not need the variablesourceRootnow. 👍@ -148,0 +206,4 @@vivado];buildPhase = ''export HOME=$(mktemp -d)You have not answered the question about which part of the build system needs $HOME.
According to the trace log:
It is found on this part of the build system:
https://github.com/elhep/linien/blob/fast_servo_merging/linien-common/linien_common/config.py#L27
Do we actually need this stuff to build the gateware or can config.py be replaced with an empty/dummy file?
I think it can be replaced with an empty/dummy file, will test and have it added to the patch.
Update: removed the
linien-commondependency (which importsconfig.py) from the gateware buildWhat exactly did you change?
Added a new commit for the changes. Converted the variables to its actual value in
linien_commonandconfig.py, so that we can omit the dependency for this use case.That's much worse than even the one line of code that sets $HOME. I had suggested to edit config.py (and not other files), what is wrong with this approach?
The config.py file that requires modification is part of the linien-common dependency:
To implement the necessary changes, we need to apply an override/overlay for the linien-common package in the upstream. Would this approach be acceptable?
You can submit a clean patch upstream so building gateware doesn't need config.py/$HOME. And in the meantime something like:
Updated: Also need to modify the file
__init__.py:For this temporary fix, it is also set to an empty file. Other approach is to create a patch that removes related lines to config.py import.
Aaah, the joys of physicist code. Removing the contents of that poorly designed
__init__.pysounds fine to me as well.@ -0,0 +20,4 @@new file mode 100644index 0000000..7c1f7a1--- /dev/null+++ b/fast-servo-linien.patch?
I don't see a fast-servo-linien.patch in
b73eea0788so this looks like your mistake.It seems I accidentally add that patch during test, apologies. Updated.
bcc12e1007to9fd509353f9fd509353ftode445b2e65@ -148,0 +195,4 @@inherit (pkgs.python3Packages.linien-common) src;prePatch = ''mkdir -p fast_servo/gatewarecp -r ${./fast-servo/linien-gateware}/. fast_servo/gateware/.should be removed I think?Removing it will cause it to create a copy of the
linien-gatewaredirectory itself and not the contents alone.ls result:
de445b2e65todf6a8233f5df6a8233f5to71d7fa227171d7fa2271toccdbd67daa@ -148,0 +210,4 @@'';doCheck = false;}))]))Indentation/style. My comment was just a hint, not a textual example.
ccdbd67daatoad725ae41b