Esp wifi/more dynamic allocations #2396

bjoernQ · 2024-10-23T11:57:16Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Prefer dynamic allocations over static allocations internally.

Testing

All examples still work

esp-wifi/src/ble/btdm.rs

esp-wifi/src/compat/common.rs

esp-wifi/src/ble/btdm.rs

bugadani

This is certainly better than before, but I'm hoping we can get rid of the huge amount of mallocs eventually.

esp-wifi/src/ble/mod.rs

esp-wifi/src/ble/btdm.rs

esp-wifi/src/ble/mod.rs

esp-wifi/src/common_adapter/common_adapter_esp32.rs

esp-wifi/src/compat/common.rs

esp-wifi/src/compat/timer_compat.rs

esp-wifi/src/esp_now/mod.rs

esp-wifi/src/ble/btdm.rs

esp-wifi/src/ble/mod.rs

esp-wifi/src/compat/common.rs

bjoernQ · 2024-10-25T15:48:07Z

This is certainly better than before, but I'm hoping we can get rid of the huge amount of mallocs eventually.

my thinking was that at least for the things indirectly allocated from the driver via malloc we currently allocate from internal-ram - we don't have control over that with the global allocator

t.b.h. I'm not sure if that would cause problems or not but if it's causing problems those things might be hard to debug

For things only allocated by our code I was fine using the global allocator (since I think these shouldn't cause problems)

bugadani · 2024-10-25T16:03:33Z

my thinking was that at least for the things indirectly allocated from the driver via malloc we currently allocate from internal-ram - we don't have control over that with the global allocator

This would be a perfect use-case for allocator-api2, that way we could define an InternalAllocator and use that instead of mallocing.

esp-wifi/src/common_adapter/mod.rs

bugadani · 2024-10-28T16:32:03Z

esp-wifi/src/compat/common.rs

            item_size,
            current_read: 0,
            current_write: 0,
            storage,
        }
    }

-    fn free_storage(&mut self) {
+    unsafe fn release_storage(&mut self) {


I appreciate this function not causing a use-after-free, but I'd appreciate at least a note that this must be called before/during dropping the queue and that the struct should probably not be used after.

Can we move this into a Drop implementation and use ptr::drop_in_place instead of calling this manually?

esp-wifi/src/esp_now/mod.rs

ProfFan · 2024-10-28T17:13:26Z

my thinking was that at least for the things indirectly allocated from the driver via malloc we currently allocate from internal-ram - we don't have control over that with the global allocator

This would be a perfect use-case for allocator-api2, that way we could define an InternalAllocator and use that instead of mallocing.

I agree, I also saw some (de)allocations happening here within critical-sections, theoretically these should ideally happen through a bounded-time allocator instead of the global allocator (if the global allocator is not bounded time). Have you considered also using a lock-free queue instead?

esp-wifi/src/ble/btdm.rs

bjoernQ · 2024-10-29T09:37:28Z

I'm not sure if I should change Vec into VecDequeue for the queues (i.e. FIFO vs LIFO)

bjoernQ · 2024-10-30T11:48:39Z

Back to draft after observing this: #2274 (comment)

bjoernQ · 2024-10-30T12:12:28Z

False alarm caused by unintendedly enabling ps-max-modem

bugadani

I stopped after about half of the changeset. I have two pain points of different severity:

core::assert will include more strings than needed
there are a few addr_of! uses that look incorrect and I'd like you to double check them. Maybe it's just github reviews being clunky, but I have low confidence in them.

esp-wifi/MIGRATING-0.10.md

esp-wifi/src/ble/btdm.rs

esp-wifi/src/ble/mod.rs

bugadani · 2024-10-30T14:23:17Z

esp-wifi/src/ble/npl.rs

-    if (*callout).dummy == 0 {
-        panic!("Trying to stop an uninitialzed callout");
-    }
+    core::assert!((*callout).dummy != 0);


Any reason these aren't the assert from fmt.rs? I'd prefer using defmt if it's enabled.

bugadani · 2024-10-30T14:26:21Z

esp-wifi/src/ble/npl.rs

+
+    let queue = (*queue).dummy as *mut ConcurrentQueue;
+    let mut event = event as usize;
+    (*queue).enqueue(addr_of_mut!(event).cast());


This looks wrong, this passes the address of the parameter (i.e. points to the stack), not the event pointer itself. Same for remove, unless I'm missing something.

Ah I see

enqueue and remove take a pointer to some data of length item_size and copy from that location into the queue.
The NPL stuff however wants us to enqueue and dequeue a pointer

(Hope that explanation makes sense - not sure how to phrase it differently)

Ehm okay I don't like this pattern but I guess it's OK because RawQueue copies out of the pointer. I'd prefer if we revisited this later and turned the queue into something typed.

Yes - RawQueue was initially done to satisfy what the WiFi driver expects from a queue - in an upcoming PR we should use something other than RawQueue here for NPL (and looking back I should have split things into separate more granular PRs)

MabezDev

This is getting a bit hard to review, so I suggest we don't add more to the PR.

Overall this a nice improvement, and I'm glad we're not storing things like the timer and rng in statics, this will allow me to do some nice things in #2301.

esp-wifi/src/common_adapter/mod.rs

esp-wifi/src/compat/common.rs

Co-authored-by: Dániel Buga <bugadani@gmail.com>

MabezDev requested changes Oct 23, 2024

View reviewed changes

esp-wifi/src/ble/btdm.rs Outdated Show resolved Hide resolved

esp-wifi/src/compat/common.rs Outdated Show resolved Hide resolved

bugadani reviewed Oct 23, 2024

View reviewed changes

esp-wifi/src/ble/btdm.rs Outdated Show resolved Hide resolved

bjoernQ force-pushed the esp-wifi/more-dynamic-allocations branch from 7649f37 to 27741c3 Compare October 24, 2024 09:40

bjoernQ marked this pull request as ready for review October 24, 2024 10:06

bjoernQ linked an issue Oct 24, 2024 that may be closed by this pull request

esp-wifi: more static's in heap memory instead #2203

Closed

bugadani reviewed Oct 25, 2024

View reviewed changes

MabezDev requested changes Oct 25, 2024

View reviewed changes

esp-wifi/src/esp_now/mod.rs Outdated Show resolved Hide resolved

esp-wifi/src/ble/btdm.rs Outdated Show resolved Hide resolved

esp-wifi/src/ble/mod.rs Outdated Show resolved Hide resolved

esp-wifi/src/compat/common.rs Outdated Show resolved Hide resolved

bjoernQ mentioned this pull request Oct 25, 2024

Make our nightly CI checks pass #2356

Merged

bjoernQ force-pushed the esp-wifi/more-dynamic-allocations branch from 7bfbea8 to d62c430 Compare October 28, 2024 13:36

bugadani reviewed Oct 28, 2024

View reviewed changes

esp-wifi/src/common_adapter/mod.rs Outdated Show resolved Hide resolved

bugadani reviewed Oct 28, 2024

View reviewed changes

ProfFan reviewed Oct 28, 2024

View reviewed changes

esp-wifi/src/esp_now/mod.rs Show resolved Hide resolved

bugadani reviewed Oct 29, 2024

View reviewed changes

esp-wifi/src/ble/btdm.rs Outdated Show resolved Hide resolved

bjoernQ marked this pull request as draft October 30, 2024 11:48

bjoernQ marked this pull request as ready for review October 30, 2024 12:11

bjoernQ force-pushed the esp-wifi/more-dynamic-allocations branch from 0bd27f4 to 928b637 Compare October 30, 2024 12:25

bugadani requested changes Oct 30, 2024

View reviewed changes

bjoernQ mentioned this pull request Oct 31, 2024

Accessing WifiEvent data #2412

Open

MabezDev approved these changes Oct 31, 2024

View reviewed changes

esp-wifi/src/common_adapter/mod.rs Show resolved Hide resolved

esp-wifi/src/compat/common.rs Show resolved Hide resolved

bugadani approved these changes Oct 31, 2024

View reviewed changes

bjoernQ added 4 commits October 31, 2024 12:58

Dynamic allocation of timers

e611f82

Replace statically allocated NPL events

02b0bc1

Remove NPL CALLOUTS

583ff73

Remove NPL EventQueue

6ea93b4

bjoernQ and others added 24 commits October 31, 2024 12:58

MSRV

1d0f3a1

Move locking into RawQueue

b11e293

Remove unused import

cd8287f

Simplify queue_recv

af15488

Address TODO

4e803df

Clippy

0641ab3

Review I

965c829

Review II

1fc3572

Clarify

358ac57

Review III

45aa0da

Review IV

fc50571

Review V

6b7d0d3

Fix

0c83691

steal RNG and RADIO_CLOCKS

6c96917

drop_in_place

06482b8

De-duplicate

dad5121

Use VecDeque

d99e218

Revert unintentional changes

aa28e99

Fix

e4bdd57

Update esp-wifi/MIGRATING-0.10.md

9509d26

Co-authored-by: Dániel Buga <bugadani@gmail.com>

assert

14530d2

Fix

cccf6ac

Expect *mut ConcurrentQueue where applicable

dc818fc

Comment why stealing is fine

43c6151

bjoernQ force-pushed the esp-wifi/more-dynamic-allocations branch from c9c42e4 to 43c6151 Compare October 31, 2024 11:58

MabezDev enabled auto-merge October 31, 2024 11:59

Fix

033a42f

MabezDev added this pull request to the merge queue Oct 31, 2024

Merged via the queue into esp-rs:main with commit e5b923b Oct 31, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Esp wifi/more dynamic allocations #2396

Esp wifi/more dynamic allocations #2396

bjoernQ commented Oct 23, 2024 •

edited

Loading

bugadani left a comment

bjoernQ commented Oct 25, 2024 •

edited

Loading

bugadani commented Oct 25, 2024

bugadani Oct 28, 2024

ProfFan commented Oct 28, 2024

bjoernQ commented Oct 29, 2024

bjoernQ commented Oct 30, 2024

bjoernQ commented Oct 30, 2024

bugadani left a comment

bugadani Oct 30, 2024

bugadani Oct 30, 2024

bjoernQ Oct 30, 2024

bugadani Oct 30, 2024 •

edited

Loading

bjoernQ Oct 30, 2024

MabezDev left a comment

Esp wifi/more dynamic allocations #2396

Esp wifi/more dynamic allocations #2396

Conversation

bjoernQ commented Oct 23, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

bugadani left a comment

Choose a reason for hiding this comment

bjoernQ commented Oct 25, 2024 • edited Loading

bugadani commented Oct 25, 2024

bugadani Oct 28, 2024

Choose a reason for hiding this comment

ProfFan commented Oct 28, 2024

bjoernQ commented Oct 29, 2024

bjoernQ commented Oct 30, 2024

bjoernQ commented Oct 30, 2024

bugadani left a comment

Choose a reason for hiding this comment

bugadani Oct 30, 2024

Choose a reason for hiding this comment

bugadani Oct 30, 2024

Choose a reason for hiding this comment

bjoernQ Oct 30, 2024

Choose a reason for hiding this comment

bugadani Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

bjoernQ Oct 30, 2024

Choose a reason for hiding this comment

MabezDev left a comment

Choose a reason for hiding this comment

bjoernQ commented Oct 23, 2024 •

edited

Loading

bjoernQ commented Oct 25, 2024 •

edited

Loading

bugadani Oct 30, 2024 •

edited

Loading