MSP430 Improvements #401

cr1901 · 2019-12-23T04:30:46Z

This PR brings MSP430 PAC generation up to parity with Cortex M PAC generation. Among other things, a device.x file is now generated for each MSP430 PAC, and the interrupt! macro has been replaced with an attribute macro from the not-yet-published msp430-rt v0.2.0.

The remaining changes are docs and cleanups of the generated code; a number of features have become stable. Only abi_msp430_interrupt is not stable yet, and I have a workaround for this that I've not written yet.

Switching to an interrupt attribute macro is a breaking change, so I incremented the "major" version number.

rust-highfive · 2019-12-23T04:30:50Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Emilgardis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Emilgardis

🎉🎉🎉

This breaks svd2rust-regress, specifically, https://github.com/cr1901/svd2rust/blob/9f4b0a357f83323165a7ab29cb2e811035f2f9a8/ci/svd2rust-regress/src/svd_test.rs#L184 needs to be done for MSP430, also L176 needs to be factored out.

Going to just comment on this as I don't feel like I can say enough about MSP430, I added one of the @rust-embedded/msp430 team members to review.

ping @awygle @pftbest

Emilgardis · 2019-12-23T07:21:41Z

src/lib.rs

+//! version = "0.2.0"
+//!
+//! [features]
+//! rt = ["msp430-rt/device"]


Haven't tested this but I think this needs to be added to svd2rust-regress

Could you elaborate on what you mean by "adding this to sv2rust-regress"?

@Emilgardis Okay, I think I have a grasp on this now.

I fixed the regression tests so the msp430 test passes. As of svd2rust 17.0 plus this PR (which actually should probably be 18.0 since this is a breaking change), svd2rust will generate a PAC that is not compatible with msp430-rt v0.1.x. Specifically:

INTERRUPTS array has been renamed __INTERRUPTS; this matters to the linker script provided with msp430-rt.

interrupts! macro replaces with attribute macro.

Right now, it seems nothing enables the rt feature of any PACs (lib.rs) generated by the regression suite. Consequently, adding msp430-rt v0.1.0 as a dep still passes- it's basically a no-op. Same situation would happen if I decided to publish and use msp430-rt v0.2.0 as a prereq for this PR.

Since I'm going to be upgrading msp430 packages in lockstep very soon, and it seems that cortex-m is also affected by not enabling the rt feature, do you object if we hold off on adding this functionality to svd2rust-regress for now?

Fine for me 👍

cr1901 · 2019-12-27T00:52:39Z

@Emilgardis (X-post from IRC) How did you specifically run svd2rust-regress (command line options) to break my PR? I've never actually used the regression suite before, so I'm unsure of what to look for.

cr1901 · 2019-12-27T21:28:43Z

@Emilgardis Could you please request a review from @awygle as well?

We also found an issue that will prevent us from merging as-is. There appears to be a bug somewhere between what TI provides us (.dslite files) and SVD generation of interrupt vectors.

I'll have to think about how to properly handle this...

YuhanLiin · 2019-12-28T22:38:17Z

Running cargo run -- -a Msp430 -v in the svd2rust-regress directory fails with Expected content in <enumeratedValues> tag, found none. This happens because the MSP430 SVD files used for regression tests are outdated and need to be regenerated.

pftbest · 2019-12-28T22:59:33Z

@YuhanLiin

svd2rust-regress has old link in the code

Here is a proper link for updated svd file:

https://raw.githubusercontent.com/pftbest/msp430g2553/v0.1.3-svd/msp430g2553.svd

YuhanLiin · 2019-12-29T15:47:07Z

After updating the SVD file, svd2rust-regress for MSP430 fails with error[E0554]: #![feature] may not be used on the stable release channel on the line #![cfg_attr(feature = "const-fn", feature(const_fn))]. This happens on master as well.

cr1901 · 2019-12-30T05:06:55Z

After updating the SVD file, svd2rust-regress for MSP430 fails with error[E0554]: #![feature] may not be used on the stable release channel on the line #![cfg_attr(feature = "const-fn", feature(const_fn))]. This happens on master as well.

I make no guarantees that msp430 0.1.x or msp430-rt 0.1.x compile on stable. If svd2rust-regress must be run on stable, this means that my svd2rust improvements rely on msp430 0.2.0 being published.

I had intended all the crates (msp430, msp430-rt, PACs) to be updated in lockstep after the svd2rust changes were merged. I wanted to wait to update the crates just in case some bugs in the msp430 crates surfaced while I made changes to svd2rust. This introduces a circular dependency.

Not sure how to proceed here. I guess I should publish msp430 0.2.0 now to shut up svd2rust-regress and release a patch version if it becomes apparent there are bugs.

Emilgardis · 2019-12-30T06:18:56Z

@cr1901 I think that's fine, don't push the 0.2.0 unless it feels like it's ready. svd2rust-regress will choose whatever toolchain is being used by default, but it can also use nightly if passed --toolchain=nightly.

burrbull · 2020-01-01T07:38:26Z

Rebase and squash your changes instead of merge master.

cr1901 · 2020-01-01T08:57:05Z

@burrbull Done. I kept the old branch locally in case it becomes necessary to use it again.

burrbull · 2020-01-01T09:07:25Z

bors try

burrbull · 2020-01-01T09:08:43Z

src/generate/interrupt.rs

                #[no_mangle]
                #[used]
-                pub static INTERRUPTS:
+                pub static __INTERRUPTS:


How will this affect other architectures?

I changed the name for parity with CortexM. It doesn't affect other architectures because the match arm only runs when the arch is Msp430. The name should've been changed a while ago, but I only got around to it now.

bors · 2020-01-01T09:46:04Z

try

Build succeeded

And happy new year from bors! 🎉

continuous-integration/travis-ci/push

therealprof

LGTM

therealprof · 2020-01-01T12:26:55Z

bors r+

bors · 2020-01-01T12:26:58Z

👎 Rejected by label

therealprof · 2020-01-01T12:29:03Z

bors r+

401: MSP430 Improvements r=therealprof a=cr1901 This PR brings MSP430 PAC generation up to parity with Cortex M PAC generation. Among other things, a `device.x` file is now generated for each MSP430 PAC, and the `interrupt!` macro has been replaced with an attribute macro from the not-yet-published `msp430-rt v0.2.0`. The remaining changes are docs and cleanups of the generated code; a number of features have become stable. Only `abi_msp430_interrupt` is not stable yet, and I have a workaround for this that I've not written yet. Switching to an `interrupt` attribute macro is a breaking change, so I incremented the "major" version number. Co-authored-by: William D. Jones <[email protected]>

burrbull · 2020-01-01T12:45:30Z

Format with cargo +stable fmt, please.

therealprof · 2020-01-01T12:50:19Z

@burrbull Is it possible to add a "fast fail" attribute to the rustfmt check only?

bors · 2020-01-01T12:53:44Z

Build failed

continuous-integration/travis-ci/push

burrbull · 2020-01-01T13:05:16Z

@burrbull Is it possible to add a "fast fail" attribute to the rustfmt check only?

You mean stop all jobs when rustfmt fails?
I'm not an expert in travis.

….2.x.

cr1901 · 2020-01-01T13:17:32Z

@burrbull Pushed w/ formatting changes. Github decided for me to dismiss your review- I didn't do it.

therealprof · 2020-01-01T13:22:57Z

bors r+

401: MSP430 Improvements r=therealprof a=cr1901 This PR brings MSP430 PAC generation up to parity with Cortex M PAC generation. Among other things, a `device.x` file is now generated for each MSP430 PAC, and the `interrupt!` macro has been replaced with an attribute macro from the not-yet-published `msp430-rt v0.2.0`. The remaining changes are docs and cleanups of the generated code; a number of features have become stable. Only `abi_msp430_interrupt` is not stable yet, and I have a workaround for this that I've not written yet. Switching to an `interrupt` attribute macro is a breaking change, so I incremented the "major" version number. Co-authored-by: William D. Jones <[email protected]>

bors · 2020-01-01T14:22:57Z

Build succeeded

And happy new year from bors! 🎉

continuous-integration/travis-ci/push

cr1901 requested a review from a team as a code owner December 23, 2019 04:30

rust-highfive assigned Emilgardis Dec 23, 2019

rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Dec 23, 2019

This was referenced Dec 23, 2019

Version 0.2.0 pftbest/msp430g2553#4

Merged

Potential update to v0.2.0 YuhanLiin/msp430fr2355-quickstart#4

Closed

Emilgardis requested a review from pftbest December 23, 2019 07:08

Emilgardis reviewed Dec 23, 2019

View reviewed changes

Emilgardis requested a review from a team December 23, 2019 07:28

Emilgardis requested a review from awygle December 28, 2019 14:07

Emilgardis added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 28, 2019

cr1901 force-pushed the msp-fix branch from 622bf25 to 4eed932 Compare January 1, 2020 08:55

burrbull reviewed Jan 1, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Jan 1, 2020

Try #401:

4c60607

burrbull previously approved these changes Jan 1, 2020

View reviewed changes

therealprof previously approved these changes Jan 1, 2020

View reviewed changes

therealprof removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 1, 2020

Update msp430 PAC generation to be compatible with msp430/msp430-rt 0…

783fbd0

….2.x.

cr1901 dismissed stale reviews from therealprof and burrbull via 783fbd0 January 1, 2020 13:14

cr1901 force-pushed the msp-fix branch from 4eed932 to 783fbd0 Compare January 1, 2020 13:14

therealprof approved these changes Jan 1, 2020

View reviewed changes

bors bot merged commit 783fbd0 into rust-embedded:master Jan 1, 2020

MSP430 Improvements #401

MSP430 Improvements #401

Uh oh!

Conversation

cr1901 commented Dec 23, 2019

Uh oh!

rust-highfive commented Dec 23, 2019

Uh oh!

Emilgardis left a comment

Choose a reason for hiding this comment

Uh oh!

Emilgardis Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

cr1901 Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

cr1901 Jan 1, 2020

Choose a reason for hiding this comment

Uh oh!

Emilgardis Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

cr1901 commented Dec 27, 2019

Uh oh!

cr1901 commented Dec 27, 2019

Uh oh!

YuhanLiin commented Dec 28, 2019

Uh oh!

pftbest commented Dec 28, 2019

Uh oh!

YuhanLiin commented Dec 29, 2019

Uh oh!

cr1901 commented Dec 30, 2019

Uh oh!

Emilgardis commented Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burrbull commented Jan 1, 2020

Uh oh!

cr1901 commented Jan 1, 2020

Uh oh!

burrbull commented Jan 1, 2020

Uh oh!

burrbull Jan 1, 2020

Choose a reason for hiding this comment

Uh oh!

cr1901 Jan 1, 2020

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jan 1, 2020

try

Build succeeded

Uh oh!

therealprof left a comment

Choose a reason for hiding this comment

Uh oh!

therealprof commented Jan 1, 2020

Uh oh!

bors bot commented Jan 1, 2020

Uh oh!

therealprof commented Jan 1, 2020

Uh oh!

burrbull commented Jan 1, 2020

Uh oh!

therealprof commented Jan 1, 2020

Uh oh!

bors bot commented Jan 1, 2020

Build failed

Uh oh!

burrbull commented Jan 1, 2020

Uh oh!

cr1901 commented Jan 1, 2020

Uh oh!

therealprof commented Jan 1, 2020

Uh oh!

bors bot commented Jan 1, 2020

Build succeeded

Uh oh!

Reviewers

Emilgardis commented Dec 30, 2019 •

edited

Loading