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

features: adds rt feature for gd32vf103-pac/rt #54

Merged
merged 1 commit into from
May 24, 2023

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented May 21, 2023

Adds a rt feature to allow users to opt-in to using the gd32vf103-pac/rt feature. This exposes the interrupt macro, allowing users to define functions for system interrupts.

This is preferable to using inline assembly to include the code in eclic-mode-hack.S which causes symbol collisions with user-defined interrupt functions, and strange bugs for handler execution.

@Disasm
Copy link
Member

Disasm commented May 21, 2023

No inline assembly is needed to define an interrupt handler: see https://github.com/riscv-rust/longan-nano/blob/master/examples/interrupt.rs for example. But using the macro is indeed a bit safer way of declaring an interrupt handler. eclic-mode-hack.S is still needed though since it implements interrupt dispatch. Without this code the interrupt macro just defines a function which will not be called.

@rmsyn
Copy link
Contributor Author

rmsyn commented May 21, 2023

Without this code the interrupt macro just defines a function which will not be called.

AFAICT this code isn't actually linked into the binary anywhere. I removed the code entirely, built against the gd32vf103-pac::interrupt macro, and the modified https://github.com/riscv-rust/longan-nano/blob/master/examples/interrupt.rs example still works the same. I think it is completely safe to remove eclic-mode-hack.S, assemble.sh, and gen-intr-lut.sh.

Also, actually moving the eclic-mode-hack.S code into inline assembly, which ensures that the code is included in linked library code, breaks the longan-nano interrupt example (without modification to the example). Meaning, actually using the code in eclic-mode-hack.S breaks existing working code.

@Disasm
Copy link
Member

Disasm commented May 22, 2023

It gets linked into the code by build.rs of this crate. eclic-mode-hack.S comes pre-compiled as bin/gd32vf103xx-hal.a. That's why maybe it works when you remove the code, try removing build.rs as well.

@rmsyn
Copy link
Contributor Author

rmsyn commented May 23, 2023

It gets linked into the code by build.rs of this crate. eclic-mode-hack.S comes pre-compiled as bin/gd32vf103xx-hal.a.

100% missed the precomp thing... I ended up removing build.rs, bin, and the like, compilation fails.

I have some current WIP stuff to move it to Rust here: #55

Not needed for this PR, though. Thanks for the pointers on how stuff gets linked together.

Copy link
Member

@Disasm Disasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, thank you for the PR!

@Disasm
Copy link
Member

Disasm commented May 23, 2023

Do you mind adding "rt" feature also to the GitHub Actions rules? https://github.com/riscv-rust/gd32vf103xx-hal/blob/master/.github/workflows/ci.yml#L26

Adds a `rt` feature to allow users to opt-in to using the
`gd32vf103-pac/rt` feature. This exposes the `interrupt` macro, allowing
users to define functions for system interrupts.

This is preferable to using inline assembly to include the code in
`eclic-mode-hack.S` which causes symbol collisions with user-defined
interrupt functions, and strange bugs for handler execution.
@rmsyn rmsyn force-pushed the feature/interrupt branch from 0131e99 to a53c4d4 Compare May 24, 2023 02:36
Copy link
Member

@Disasm Disasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Disasm Disasm merged commit fdb0685 into riscv-rust:master May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants