DRTIO port - scripts #135

Merged
sb10q merged 5 commits from mwojcik/artiq-zynq:drtio_scripts into master 1 year ago
mwojcik commented 1 year ago
Owner

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 1 year ago
Owner

I guess this has to be merged last?

I guess this has to be merged last?
sb10q reviewed 1 year ago
src/Makefile Outdated
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 1 year ago
Owner

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 1 year ago
Poster
Owner

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 1 year ago
Owner

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 1 year ago
Poster
Owner

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 1 year ago
Owner

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 1 year ago
Poster
Owner

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 1 year ago
src/Makefile Outdated
../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 1 year ago
Owner

Why the deviation from the runtime Make rules?

Why the deviation from the runtime Make rules?
mwojcik commented 1 year ago
Poster
Owner

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 1 year ago
Owner

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
Poster
Owner

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 1 year ago
inherit pkgs;
board = target;
};
fwtype = if builtins.elem variant sat_variants then "satman" else "runtime";
sb10q commented 1 year ago
Owner

You can read the JSON to determine that.

You can read the JSON to determine that.
sb10q commented 1 year ago
Owner

Nix has builtins.fromJSON

Nix has ``builtins.fromJSON``
mwojcik commented 1 year ago
Poster
Owner

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 1 year ago
mwojcik added 1 commit 1 year ago
sb10q reviewed 1 year ago
src/Makefile Outdated
GWARGS := -V simple
all: ../build/firmware/armv7-none-eabihf/release/runtime ../build/runtime.bin
all: runtime
sb10q commented 1 year ago
Owner

There is no rule named runtime.

There is no rule named ``runtime``.
mwojcik marked this conversation as resolved
sb10q reviewed 1 year ago
runtime_target: ../build/runtime.bin
satman_target: ../build/satman.bin
sb10q commented 1 year ago
Owner

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 1 year ago
Poster
Owner

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 1 year ago
Owner

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 1 year ago
mwojcik added 1 commit 1 year ago
sb10q merged commit 0efa83e956 into master 1 year ago
sb10q referenced this issue from a commit 1 year ago
The pull request has been merged as 0efa83e956.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.