Skip to content

Conversation

JalonWong
Copy link
Contributor

No description provided.

@JalonWong JalonWong requested a review from a team as a code owner August 14, 2025 19:33
@JalonWong JalonWong force-pushed the master branch 4 times, most recently from 6bca433 to 6335bb8 Compare August 15, 2025 19:49
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Sounds good to me, thanks! Just a bit of nitpicks and it should be mergable.

@JalonWong JalonWong force-pushed the master branch 2 times, most recently from 9951db6 to 080581e Compare August 17, 2025 05:23
@JalonWong
Copy link
Contributor Author

@zeenix I have made improvements based on the suggestions, and I added a assert!($size > 0); in the macro. Is it appropriate?

zeenix
zeenix previously approved these changes Aug 17, 2025
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zeenix
Copy link
Contributor

zeenix commented Aug 17, 2025

@JalonWong I'll merge soon but I want to first give others a chance to have a look.

CC @adamgreig @sgued

@adamgreig
Copy link
Member

My only thought is that people are likely to want to configure where the backing store for the heap is located in memory using link_section. If the assert!() wasn't there, you could put #[unsafe(link_section=".sram2")] before the init!() macro call; I'm not sure what's most elegant otherwise though:

  • add an optional macro argument for link section? helps for this case, but maybe people will want other attributes (though I think link section is the main one)
  • move the assert later so people can decorate the static mut with whatever attributes they want? I guess it works but it's kind of ugly and unclear when reading the code what it applies to
  • leave it as-is, and if someone wants to specify the linker section they can just write the macro code out by hand instead

@zeenix
Copy link
Contributor

zeenix commented Aug 18, 2025

  • add an optional macro argument for link section? helps for this case, but maybe people will want other attributes (though I think link section is the main one)
  • leave it as-is, and if someone wants to specify the linker section they can just write the macro code out by hand instead

How about a combination of these two: we add the optional argument for the linker section and if people need to add other attributes, they just don't use the macro?

@JalonWong
Copy link
Contributor Author

@zeenix @adamgreig If someone want to use link_section I think they just don't use the macro. I leave the previous example in the README.

@JalonWong JalonWong force-pushed the master branch 5 times, most recently from f447854 to d47bd8a Compare August 19, 2025 02:51
@JalonWong
Copy link
Contributor Author

JalonWong commented Aug 19, 2025

@sgued @zeenix I found out that it's very easy to add once in the init(), because critical_section is already used in it. I just added a bool member variable. This keeps the macro very simple, I also remove the unsafe in the macro, it works fine.

Do you think that this is a better solution?

@JalonWong JalonWong force-pushed the master branch 5 times, most recently from f5ba156 to b3afee2 Compare August 27, 2025 16:21
sgued
sgued previously approved these changes Aug 28, 2025
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Sorry but still some things to do. Thank you for your patience and hard work on this so far. 👍

Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

A tiny nit and then we're good to merge. 👍

Co-authored-by: Zeeshan Ali Khan <[email protected]>
@zeenix zeenix added this pull request to the merge queue Aug 31, 2025
Merged via the queue into rust-embedded:master with commit 83157f6 Aug 31, 2025
8 checks passed
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.

4 participants