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

bring up to par with cortex-m-rt v0.6.x #4

Merged
merged 8 commits into from
Dec 22, 2019
Merged

bring up to par with cortex-m-rt v0.6.x #4

merged 8 commits into from
Dec 22, 2019

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 14, 2018

  • this brings in cortex-m-rt attributes: entry, pre_init and interrupt
  • it also removes almost all dependencies on unstable features -- only the
    msp430-interrupt ABI is left behind.
  • it also removes the abort-on-panic feature and the panic_handler (panic_impl lang item)

I have a few questions / comments related to some of the changes I made

  1. I have assumed that the vector table is always 32 bytes in size. That seems to the maximum vector table for MSP430 CPUs AFAICT (MSP430X CPUs support more interrupts). Is this table size correct? Or should it be bigger to accommodate some device?

  2. Like I said the vector table is fixed size. This means that if your device has less than 15 interrupts (plus Reset) the table will still be 32 bytes in size so you won't be able to use the free space for your program. In other words, your .text + .rodata end address can not exceed 0xFFE0 even if the vector table starts at e.g. 0xFFF0.

  3. Just a comment: because the vector table is now fixed size the VECTORS memory region is no longer used and doesn't need to be included in memory.x. Instead the ROM memory region should always have end address equal to 0x1_0000 because the .vector_table section is placed at the end of ROM. (I should document this requirement better)

  4. Is there any interrupt handler that should be forbidden from returning? In Cortex-M land the HardFault handler fires on invalid memory access, execution of illegal / undefined instructions, etc. Returning from this exception would resume a program that's in an invalid state and would result in UB. For this reason we have the exception attribute enforce that the HardFault handler is a divergent function (fn() -> !). Is there anything like that in MSP430 land?

  5. A heads up: This is a breaking change that will require regenerating device PAC crates with svd2rust v0.14.0 (I need to land some MSP430 changes in svd2rust before making the v0.14.0 release)

r? @cr1901
cc @awygle

- this brings in cortex-m-rt attributes: entry, pre_init and interrupt
- it also removes almost all dependencies on unstable features -- only the
  msp430-interrupt ABI is left behind.
- it also removes the abort-on-panic feature and the panic_handler (panic_impl
  lang item)
@japaric
Copy link
Member Author

japaric commented Nov 14, 2018

  1. It seems that the NMI (interrupt at 0xFFFC) is not device specific. Should this interrupt be overridable via the interrupt attribute when the "device" feature is disabled? I think this would imply that SVD files must not include NMI in their list of interrupts.

@cr1901
Copy link
Contributor

cr1901 commented Nov 22, 2018

Some preliminary answers/responses to your q's/comments:

1./2. The MSP430F55{1,2}x family is an example of an MSP430 (not X) core with 23 interrupts, and up to 64 are reserved (page 53). Assuming you make the vector table a fixed 64 entries, that's (64-16)*2= 96 bytes lost to the vector table on small devices like the MSP430G2211 I use for AT2XT, or about 5% overhead.

  1. Ack. Trying to apply this breaks my own codebase (ERROR(msp430-rt): .vector_table is shorter than expected.), but we can talk about this on IRC.

  2. Flash memory access violation might fall under this (this can occur when writing new data (!) to the flash)?

  3. Ack.

  4. Having not kept up w/ Cortex-M land, I don't understand what you mean by "overrideable". Is the idea that if you're not targeting a specific device, you only have access to NMI b/c it's truly "generic" across all devices? (Having only access to NMI for device-independent code seems quite limiting.)

@Disasm Disasm requested a review from cr1901 February 24, 2019 09:17
@Disasm Disasm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. breaking change labels Feb 24, 2019
@cr1901
Copy link
Contributor

cr1901 commented Mar 12, 2019

@Disasm Per agreement w/ @japaric, we are waiting a few months (post-thesis) to discuss merging this.

@japaric By agreement, svd2rust 0.14.0 came without the msp430 changes. When you get a free moment- could you elaborate on what msp430-specific changes you needed to add to svd2rust as a "record" of what still needs to be done? That way, we can pick up from where we left off.

@mathk mathk added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2019
@cr1901
Copy link
Contributor

cr1901 commented Dec 16, 2019

@japaric I've started looking into bring msp430-rt up to par earnestly. I want to use this PR as a base, but there are some modifications I want to make. In particular (will add to this list):

  1. It appears that cortex-m-rtfm grew the ability to have a variable-sized vector table. I think I want that functionality here as well.

@cr1901
Copy link
Contributor

cr1901 commented Dec 20, 2019

@japaric There are some problems I need to resolve locally (along with updating documentation), but I have confirmed your changes work and I can access peripherals with the take API using a device crate I generated with svd2rust v0.16.1 plus local changes. So all in all, I'm very happy w/ the PR, even though it took me forever to review- good work :D!

Re: 1., 2. and 3. I have gone back to the old style of having the user specify a VECTORS memory region manually for now. The start address of the vector table can be extracted from TI's headers (or at least @pftbest's msp430_svd can extract the info), but I'm not sure how to best incorporate that into svd2rust at present.

I wish to punt on 4. and 6. for now and come back to them later.

Re: 5.

By agreement, svd2rust 0.14.0 came without the msp430 changes.

I'm taking care of the required changes on my end- at least, the ones required to play nice with this crate's breaking changes :).

My additions to this PR can be found here: https://github.com/cr1901/msp430-rt/tree/rt-up

@cr1901 cr1901 merged commit faee1e3 into master Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants