search

lobaro / FreeRTOS-rust

Rust crate for FreeRTOS
MIT License
354 stars 56 forks source link

Remove allocator_api feature dependency to allow stable 1.68 #50

Closed JalonWong closed 1 year ago

schteve commented 1 year ago

What changes are coming in 1.68 that make nightly not necessary? I looked for changelog notes (found these but didn't notice anything relevant).

Also, I see there was another change which switches the Linux port path - what is this for?

JalonWong commented 1 year ago

What changes are coming in 1.68 that make nightly not necessary? I looked for changelog notes (found these but didn't notice anything relevant).

Also, I see there was another change which switches the Linux port path - what is this for?

This feature was stabilized in 1.68. So no_std + liballoc applications can run on the Stable channel. Currently, 1.68 has been released to the beta channel.

We can use nightly and allocator_api only when we need to use AllocRef (which is not often used).

Switch the path because the old path no longer exists.

schteve commented 1 year ago

Sorry I don't see the connection there - as I understand, we need nightly because allocator_api is unstable - the feature you mentioned doesn't change that, right? I suppose 1.68 is releasing in a few weeks so we can always test then to see.

Also, I think for the Linux change there is some misunderstanding - GCC/Linux isn't present in the FreeRTOS port itself but for the Linux example we use a 3rd party port, in which case you need to change the port base using freertos_port_base(). So it does work right now, although admittedly maybe not as smoothly or obviously as it could be.I'm not opposed to switching over (it would be nice to stay within the FreeRTOS repo for it), but I would want to understand what benefits there are.

JalonWong commented 1 year ago

I have reverted the change of path. Although I think the default path should follow the FreeRTOS official practice. I tested the port in Linux which under ThirdParty/GCC/Posix, it works fine.

Maybe I missed something. I haven't found any code in freertos-rust that depends on allocator_api. Actually I have built and debugged the code with 1.68 beta (without nightly and allocator_api) on a STM32F1 MCU and haven't had any problems. I think we had to use nightly before because of feature alloc_error_handler.

schteve commented 1 year ago

You may be right, I'll have to go back and look at it closer when I have time. At first glance it does seem that allocator_api is for running multiple allocators and I'm not sure that's something we really need here. As long as we can set the global allocator and OOM handler.

schteve commented 1 year ago

If we decide allocator_api isn't relevant here we should update the README as well.

schteve commented 1 year ago

I think we can wait until 1.68 stable releases and then we can test there. I did check that I can build successfully when removing the line #![cfg_attr(feature = "allocator", feature(allocator_api))]. So that supports the idea that allocator_api isn't even used in the project right now.

JalonWong commented 1 year ago

I think we can wait until 1.68 stable releases and then we can test there. I did check that I can build successfully when removing the line #![cfg_attr(feature = "allocator", feature(allocator_api))]. So that supports the idea that allocator_api isn't even used in the project right now.

I think so too.

agerasev commented 1 year ago

I believe allocator feature should be removed as well.

The GlobalAlloc itself is stable, and the only issue preventing it from being used in stable was the default_alloc_error_handler, which is stabilized just in 1.68.

schteve commented 1 year ago

From my side it looks like stable 1.68 works fine when removing the allocator_api feature and I can't see a reason why we need it. So my suggestion is to update this PR to just remove the line instead of modifying it, and also update the comment below it which says nightly is needed if using the allocator.

JalonWong commented 1 year ago

From my side it looks like stable 1.68 works fine when removing the allocator_api feature and I can't see a reason why we need it. So my suggestion is to update this PR to just remove the line instead of modifying it, and also update the comment below it which says nightly is needed if using the allocator.

I have modified it. I didn't mention allocator_api in the comments because I thought it was unnecessary.

schteve commented 1 year ago

Yep no point in mentioning it now that it's removed. Thanks!