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

pkg/nimble: Update to 1.8.0 #21108

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Dec 24, 2024

Contribution description

This updates nimble, with all the adjustments that need to go with it.

Testing procedure

Issues/PRs references

Updating this is a prerequisite to using ready-made Rust wrappers on nimble (eg. https://github.com/embassy-rs/trouble/), which is preferable over safely wrapping the whole API surface just once ore.

Current status

Not getting it to build yet; we'll need some patches for upstream problems (not following IWYU) and our dead-code strictness -- and then (currently: after fixing them manually), a few symbols are missing, which should be easy to fix but is still a tedious task.

[edit: It builds now and is tested]

All patches should be upstreamed.

[edit: All applicable patches are filled upstream.]

@github-actions github-actions bot added Area: pkg Area: External package ports Area: BLE Area: Bluetooth Low Energy support labels Dec 24, 2024
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 24, 2024
@riot-ci
Copy link

riot-ci commented Dec 24, 2024

Murdock results

✔️ PASSED

16692a9 DO NOT MERGE: Disable nimble on ESP32

Success Failures Total Runtime
10222 0 10222 16m:15s

Artifacts

@chrysn
Copy link
Member Author

chrysn commented Dec 27, 2024

@sjanc, could you help me here? I'm always running into g_ble_ll_data.ll_evq being uninitialized; the place where it would be initialized is ble_ll_init, but that is called from nowhere. Should I just call that (using an exern void ble_ll_init(void) because it's not even in headers), and should it come before or after nimble_port_init?

@github-actions github-actions bot added Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Dec 27, 2024
chrysn added a commit to RIOT-OS/rust-riot-sys that referenced this pull request Dec 28, 2024
These are more instances where [2221] bites, which will be introduced
when RIOT updates to Nimble 1.8 (as is being worked on in [21108])

[2221]: rust-lang/rust-bindgen#2221
[21108]: RIOT-OS/RIOT#21108
@sjanc
Copy link

sjanc commented Jan 2, 2025

@sjanc, could you help me here? I'm always running into g_ble_ll_data.ll_evq being uninitialized; the place where it would be initialized is ble_ll_init, but that is called from nowhere. Should I just call that (using an exern void ble_ll_init(void) because it's not even in headers), and should it come before or after nimble_port_init?

ble_ll_init() is initializing controller part and on Mynewt it is called from generated sysinit calls, so yes, you'll have to use extern in port (at least for now). It shall be called before ble_transport_hs_init() and ble_transport_ll_init() calls [1][2]

I think you should update nimble_port_init() to adjust for those changes as controller port is very HW/OS specific...
So call to ble_ll_init() should be from nimble_port_init() with proper orders to keep init order requirements.

[1] https://github.com/apache/mynewt-nimble/blob/master/nimble/controller/pkg.yml#L45
[2] apache/mynewt-nimble#1648

@chrysn
Copy link
Member Author

chrysn commented Jan 4, 2025

Thanks; moved into a patch while we're aiming for 1.8.0, and filed as apache/mynewt-nimble#1956. All patches that are now applied are submitted upstream (one even already merged), with the exception of a dead code one where I'm not sure whether this shouldn't better be handled by a less pedantic flag set through our build system.

Builds pass, and I've not only used this with the heart rate example described in tests, but also to set up an IPSP interface.

I'm placing this as ready-for-review -- if the ESP thing can be sorted out quickly, I have some remaining hope to get this in before soft freeze.

@chrysn chrysn marked this pull request as ready for review January 4, 2025 13:46
@chrysn chrysn requested a review from gschorcht as a code owner January 4, 2025 13:46
@chrysn chrysn requested review from maribu and bergzand January 4, 2025 13:51
@chrysn
Copy link
Member Author

chrysn commented Jan 4, 2025

Seems the ESP port is maintained by the ESP vendor, who announced updating weeks ago, but we'll need an ESP update.

@maribu
Copy link
Member

maribu commented Jan 6, 2025

I recall that for lvgl we had two packages for some time when upstream did a major release (with breaking API changes).

I wonder if that approach to keep both versions as separate packages and let ESP only work with the old one would be the way to unblock this here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants