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

Board based instantiation of chip drivers and interrupt mappings: Stm32f4 #2188

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

hudson-ayers
Copy link
Contributor

Pull Request Overview

This pull request ports the stm32f4xx, stm32f446re, stm32f412g, stm32f429zi chips and the boards that use them to the new peripheral instantiation approach that does not rely on global variables (first proposed in #2069).

This involved mostly the same changes as were required for the stm32f3xx chip, with some additional changes for handling DMA, because there is tons of duplicate code across those chips and boards. It would be nice if those chips and boards had an approach to code sharing more similar to that used by the nordic boards, but that would be a substantial refactor so not sure if its worth it for anyone to take that on.

This PR removes a ton of unsafe, which is nice. It also adds support for different stm chips to support different peripherals (in much the same way the nordic chips handle this) which should help for #2182 .

Testing Strategy

This pull request was tested by compiling. But there are enough changes in this PR that hardware testing would be appreciated.

TODO or Help Wanted

N/A

Documentation Updated

  • No updates are required.

Formatting

  • Ran make prepush.

@hudson-ayers hudson-ayers added refactor stm32 Change pertains to the stm32 family of MCUSs labels Nov 6, 2020
@bradjc bradjc merged commit 6647dd0 into tock:master Nov 13, 2020
@hudson-ayers hudson-ayers mentioned this pull request Nov 13, 2020
2 tasks
Comment on lines -43 to +44
let uart = unsafe { &mut stm32f412g::usart::USART2 };
let rcc = stm32f412g::rcc::Rcc::new();
let uart = stm32f412g::usart::Usart::new_usart2(&rcc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't uart still be a mutable reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now recreated as an owned type, which means it is still mutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this particular use of UART is only for use by the panic handler

Copy link
Contributor

@krady21 krady21 Nov 17, 2020

Choose a reason for hiding this comment

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

I was looking into io.rs because the panic handler doesn't seem to work either. I got this when I called panic!, so I thought that the problem might be related to usart. I couldn't seem to find anything wrong in the initialization procedure, the only thing different from before being the order in which setup_peripherals(), set_pin_primary_functions(), setup_dma() are called in the reset handler. I tried reordering them, but I got the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, that suggests that the usart speed might be wrong. Where were you calling panic!()? Was it panicking even when you did not call panic!()? Do normal console prints (debug!()) work?

Copy link
Contributor

@alexandruradovici alexandruradovici Nov 18, 2020

Choose a reason for hiding this comment

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

I finally got the panic working, it seems to get stuck at this line

let gpio_ports = stm32f412g::gpio::GpioPorts::new(&rcc, &exti);

If loaded with apps, it fails in reset_handler at

let peripherals = static_init!(
        Stm32f412gDefaultPeripherals,
        Stm32f412gDefaultPeripherals::new(rcc, exti, dma1)
    );

It actually triggers a hard fault. Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you can't get the details of the hard fault because it panics too soon? Does whatever debugger you are using report any additional information about the type of fault?

When you say you got the panic working, what did you change to make it work?

I don't immediately have any ideas, as the constructor code is pretty simple. I am going to look into this more though. I'm really sorry this refactor broke things; for all the hardware I had the refactors simply worked, but I don't have an stm board and something must be different here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you try doubling (or even tripling?) the stack size, and seeing if you still encounter the same issue? The stm32f4 chip has 6280 bytes of peripherals, which are now being lazily initialized. I suspect that because they are all being initialized in a single function call (within the static_init!() macro), that all 6280 bytes may be being allocated on the stack simultaneously as part of the initialization, thereby overflowing the stack.

If that is what is happening, it is probably a flaw with this design, and I should break the initialization into smaller pieces to avoid actually requiring a larger stack. But just increasing the stack size should be the quickest way to determine if that is in fact the issue. Generally, the kernel should report stack overflow errors as such, but I wonder if stack overflows this early in the initialization sequence lead to other issues with uninitialized peripherals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did it, this is the problem. Sent #2223

bors bot added a commit that referenced this pull request Nov 27, 2020
2223: Stack size update for STM32F4 boards r=hudson-ayers a=alexandruradovici

### Pull Request Overview

This pull request fixes the stack size requirement for STM32F4 boards. Due to the new board instantiation #2188, the required stack size has to be doubled as @hudson-ayers suggested in #2188 (comment).

### Testing Strategy

This pull request was tested using an STM32f412GDiscovery board.

### TODO or Help Wanted

@krady21, @cosmindanielradu19, @teonaseverin  please test using the ~STM32F3Discovery and~ Nucleo. Running the blink app is just enough.

### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor stm32 Change pertains to the stm32 family of MCUSs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants