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

Add SMT32WB55 and STM32L475 support #1052

Closed
wants to merge 11 commits into from
Closed

Conversation

jnthbdn
Copy link

@jnthbdn jnthbdn commented Sep 22, 2023

We needed DapLink support (mainly for the WebUSB) for the STM32L475 and STM32WB55 chips, so I added it.

I hope the PR is as expected. Do I need to specify anything else?

Thank you,
Jonathan

@mbrossard
Copy link
Contributor

Hi,

I created a branch pr/letssteam with a rework of your PR:

  • separated the two boards into separate commits and squashed some the minor changes.
  • removed the changes to generic parts of DAPLink.
    • 4a6fa55: I agree it's cleaner, but I would rather not see random small clean-ups like that.
    • d97217e: this change would affect all targets. If the board flags do not set kEnablePageErase erase_chip() should already called (see here).
    • 9949eee: the source/rtos/cmsis_os2_port.c is only used for armcc compiler.
  • left out some part of your board specific changes:
    • 6e0599e: I don't think those changes have any effect.
    • dcb3399: it is surprising that you increased the stack size, I am unsure why those boards would need this.
  • added changes to add the CMSIS DAP v2.1 extra fields and updated the flash algos from the latest CMSIS packs using the DAPLink script. Those were additional steps that were new so I updated the documentation.

Let me know what you think.

@jnthbdn
Copy link
Author

jnthbdn commented Sep 25, 2023

Hi,

Thanks for your reply.
I understand why you have discarded certain commits, no problem for me, except for 9949eee, the problem is that we use the armcc compiler, and without this modification, I don't know how to modify the stack size (but I've certainly missed something).

Speaking of the stack size, it's been almost a year since we made these changes, and I'll admit I can't remember the exact reason why we needed them, but I remember very well that this was the solution 😄.

@mbrossard
Copy link
Contributor

the problem is that we use the armcc compiler, and without this modification, I don't know how to modify the stack size (but I've certainly missed something).

You are right about that. I added this commit 0d50d0b will make MAIN_TASK_STACK and align its value with rtos2.

Speaking of the stack size, it's been almost a year since we made these changes, and I'll admit I can't remember the exact reason why we needed them, but I remember very well that this was the solution 😄.

I was hoping you would remember the failure case. With this new change stack size goes from 800 to 864. Could you check if that is enough?

@jnthbdn
Copy link
Author

jnthbdn commented Sep 26, 2023

I'll try that on our targets. And see if the problem reappears.

But I wonder if it wouldn't be simpler (or more practical) to have a way of forcing the stack size only for a specific target. I understand that the system of redefining the #define as I propose in the PR isn't optimal, but I find it radical to modify the stack size for all targets if only one needs a little more space, don't you?

I'll get back to you as soon as I've done my tests.

(Sorry about the push force, I committed to the wrong branch ^^)

@mbrossard
Copy link
Contributor

But I wonder if it wouldn't be simpler (or more practical) to have a way of forcing the stack size only for a specific target. I understand that the system of redefining the #define as I propose in the PR isn't optimal, but I find it radical to modify the stack size for all targets if only one needs a little more space, don't you?

You can the modification on a per-board basis by setting the MAIN_TASK_STACK value in the board record file (the same way you set USB_PROD_STR).

@mathias-arm
Copy link
Collaborator

Fixed in dbd02db

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.

3 participants