M-Labs/artiq-zynq
10
1
Fork 19
Code Issues 12 Pull Requests Releases Wiki Activity

DRTIO port - scripts #135

Merged
sb10q merged 5 commits from mwojcik/artiq-zynq:drtio_scripts into master 2021-10-08 16:25:13 +08:00
Conversation 2 Commits 5 Files Changed 7 +171 -18
mwojcik commented 2021-10-05 16:12:38 +08:00
Owner
Copy Link

This is a part of few pull requests for DRTIO functionality. They have been broken up to allow easier code review overall. They're identical in contents to drtio_port branch.

This PR includes modified makefile, nix and shell scripts for building and running the firmware.

This PR should be merged last.

Any feedback and suggestions are welcome.

This is a part of few pull requests for DRTIO functionality. They have been broken up to allow easier code review overall. They're identical in contents to drtio_port branch. This PR includes modified makefile, nix and shell scripts for building and running the firmware. This PR should be merged last. Any feedback and suggestions are welcome.
mwojcik added 1 commit 2021-10-05 16:12:38 +08:00
sb10q commented 2021-10-05 16:16:55 +08:00
Owner
Copy Link

I guess this has to be merged last?

I guess this has to be merged last?
sb10q reviewed 2021-10-05 16:17:53 +08:00
src/Makefile Outdated
@ -12,2 +13,3 @@
python gateware/$(TARGET).py -r ../build/pl.rs -c ../build/rustc-cfg -m ../build/mem.rs $(GWARGS)
../build/firmware/armv7-none-eabihf/release/runtime: ../build/pl.rs ../build/rustc-cfg $(shell find . -print)
../build/firmware/armv7-none-eabihf/release/runtime: ../build/pl.rs ../build/rustc-cfg
sb10q commented 2021-10-05 16:17:53 +08:00
Owner
Copy Link

Doesn't this break dependency checks? If you modify a source file Make won't re-run the build properly anymore.

Doesn't this break dependency checks? If you modify a source file Make won't re-run the build properly anymore.
mwojcik commented 2021-10-05 16:32:27 +08:00
Author
Owner
Copy Link

No, it doesn't from what I saw. Cargo must do the checks on its own. And with find . it would also try to build satman at the same time as runtime.

No, it doesn't from what I saw. Cargo must do the checks on its own. And with ``find .`` it would also try to build satman at the same time as runtime.
sb10q commented 2021-10-05 16:34:52 +08:00
Owner
Copy Link

Yes. But the Makefile needs to run Cargo at all when source is modified.
AFAICT if you remove the find then Make will simply skip running Cargo after a corresponding source file is modified.

Yes. But the Makefile needs to run Cargo at all when source is modified. AFAICT if you remove the ``find`` then Make will simply skip running Cargo after a corresponding source file is modified.
mwojcik commented 2021-10-05 16:54:24 +08:00
Author
Owner
Copy Link

Well, with it, it tries to built satman with pl/mem files obviously not prepared, failing the build. And makes a circular dependency.

Calling make for master runtime throws a warning, and then fails when trying to build satman.

make: Circular ../build/firmware/armv7-none-eabihf/release/satman <- satman dependency dropped.
make: Circular ../build/firmware/armv7-none-eabihf/release/satman <- runtime dependency dropped.

I haven't had issues with modifying a file and not compiling, either. Only when gateware was changed, then yes, it couldn't detect that, and required a clean build.

Well, with it, it tries to built satman with pl/mem files obviously not prepared, failing the build. And makes a circular dependency. Calling make for master runtime throws a warning, and then fails when trying to build satman. ``` make: Circular ../build/firmware/armv7-none-eabihf/release/satman <- satman dependency dropped. make: Circular ../build/firmware/armv7-none-eabihf/release/satman <- runtime dependency dropped. ``` I haven't had issues with modifying a file and not compiling, either. Only when gateware was changed, then yes, it couldn't detect that, and required a clean build.
sb10q commented 2021-10-05 17:02:41 +08:00
Owner
Copy Link

I haven't had issues with modifying a file and not compiling, either.

How did Make detect the change then?

> I haven't had issues with modifying a file and not compiling, either. How did Make detect the change then?
mwojcik commented 2021-10-05 17:48:00 +08:00
Author
Owner
Copy Link

Good question, actually. It's just something I wasn't really thinking about as long as it could compile. A bit of an online search only says "it resolves the dependencies" somehow, I haven't went deep into it.

For circular dependencies and miscompilations, of course that's my mistake - I made targets runtime/satman so nix-build could refer to these, but of course they're listed along with find, causing these issues. I'll change their name so they don't collide, update build script and restore find.

Good question, actually. It's just something I wasn't really thinking about as long as it could compile. A bit of an online search only says "it resolves the dependencies" somehow, I haven't went deep into it. For circular dependencies and miscompilations, of course that's my mistake - I made targets runtime/satman so nix-build could refer to these, but of course they're listed along with ``find``, causing these issues. I'll change their name so they don't collide, update build script and restore ``find``.
sb10q reviewed 2021-10-05 16:18:24 +08:00
src/Makefile Outdated
@ -20,2 +22,4 @@
../build/runtime.bin: ../build/firmware/armv7-none-eabihf/release/runtime
llvm-objcopy -O binary ../build/firmware/armv7-none-eabihf/release/runtime ../build/runtime.bin
satmanout: ../build/pl.rs ../build/rustc-cfg
sb10q commented 2021-10-05 16:18:24 +08:00
Owner
Copy Link

Why the deviation from the runtime Make rules?

Why the deviation from the runtime Make rules?
mwojcik commented 2021-10-05 16:33:29 +08:00
Author
Owner
Copy Link

You mean in the name of the rule? Fair point, no particular reason for it, I'll revert it.

You mean in the name of the rule? Fair point, no particular reason for it, I'll revert it.
sb10q commented 2021-10-05 16:36:09 +08:00
Owner
Copy Link

The name of the rule needs to match the output file, otherwise Make keeps evaluating it every time it is invoked with that target.

The name of the rule needs to match the output file, otherwise Make keeps evaluating it every time it is invoked with that target.
mwojcik marked this conversation as resolved
mwojcik commented 2021-10-05 16:18:53 +08:00
Author
Owner
Copy Link

Yes, I think so. Good point, I'll make note of order of merging.

Yes, I think so. Good point, I'll make note of order of merging.
sb10q reviewed 2021-10-05 16:19:50 +08:00
default.nix
@ -14,3 +15,4 @@
inherit pkgs;
board = target;
};
fwtype = if builtins.elem variant sat_variants then "satman" else "runtime";
sb10q commented 2021-10-05 16:19:50 +08:00
Owner
Copy Link

You can read the JSON to determine that.

You can read the JSON to determine that.
sb10q commented 2021-10-05 16:20:31 +08:00
Owner
Copy Link

Nix has builtins.fromJSON

Nix has ``builtins.fromJSON``
mwojcik commented 2021-10-05 16:36:16 +08:00
Author
Owner
Copy Link

zc706 doesn't use JSONs, though. Now that I think about it I could make it a bit better still by having the fwtype specified in the list of targets at the bottom, passed to the build function instead.

zc706 doesn't use JSONs, though. Now that I think about it I could make it a bit better still by having the fwtype specified in the list of targets at the bottom, passed to the build function instead.
mwojcik added 1 commit 2021-10-05 16:55:54 +08:00
mwojcik added 1 commit 2021-10-05 17:59:39 +08:00
sb10q reviewed 2021-10-07 08:31:37 +08:00
src/Makefile Outdated
@ -2,3 +2,3 @@
GWARGS := -V simple
all: ../build/firmware/armv7-none-eabihf/release/runtime ../build/runtime.bin
all: runtime
sb10q commented 2021-10-07 08:31:37 +08:00
Owner
Copy Link

There is no rule named runtime.

There is no rule named ``runtime``.
mwojcik marked this conversation as resolved
sb10q reviewed 2021-10-07 08:32:24 +08:00
src/Makefile
@ -5,0 +5,4 @@
runtime_target: ../build/runtime.bin
satman_target: ../build/satman.bin
sb10q commented 2021-10-07 08:32:24 +08:00
Owner
Copy Link

What are these *_target things for?
Also (assuming they are necessary at all), if there are no *_target files produced those rules should be PHONY.

What are these ``*_target`` things for? Also (assuming they are necessary at all), if there are no ``*_target`` files produced those rules should be ``PHONY``.
mwojcik commented 2021-10-07 15:40:05 +08:00
Author
Owner
Copy Link

I was aiming for clearer reference of the targets in nix-scripts, or when called manually. But it's unnecessary after all, it can be called straight up with the output file rule.

I was aiming for clearer reference of the targets in nix-scripts, or when called manually. But it's unnecessary after all, it can be called straight up with the output file rule.
sb10q commented 2021-10-08 16:24:48 +08:00
Owner
Copy Link

OK, I see. Yes the manual command is a bit unwieldy, I suggest adding them back but listed as PHONY. I will merge this now, please send a new PR.

OK, I see. Yes the manual command is a bit unwieldy, I suggest adding them back but listed as PHONY. I will merge this now, please send a new PR.
mwojcik added 1 commit 2021-10-07 15:40:16 +08:00
mwojcik added 1 commit 2021-10-07 20:48:18 +08:00
sb10q merged commit 0efa83e956 into master 2021-10-08 16:25:13 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#135
No description provided.
Delete Branch "mwojcik/artiq-zynq:drtio_scripts"

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?