BLE Refactor - Skeleton code implementation#880
BLE Refactor - Skeleton code implementation#880arun-silabs wants to merge 3 commits intofeature/ble_refactorfrom
Conversation
|
bugbot run |
Bugbot couldn't runBugbot is not enabled for your user on this team. Ask your team administrator to increase your team's hard limit for Bugbot seats or add you to the allowlist in the Cursor dashboard. |
| "${chip_root}/src/platform:platform_base", | ||
|
|
||
| # BLE refactor skeleton (Matter channel) | ||
| "${silabs_platform_dir}/ble:ble", |
There was a problem hiding this comment.
can't this be part of ${silabs_platform_dir}/ble/SiWx:ble-siwx and ${silabs_platform_dir}/ble/efr32:ble-efr32
| virtual bool CanHandleEvent(uint32_t eventId) { return false; } | ||
| virtual void ParseEvent(void * platformEvent) {} | ||
|
|
||
| // ----- CLI: GAP ----- |
There was a problem hiding this comment.
why CLI ?
| virtual CHIP_ERROR SetCharacteristicNotification(uint8_t characteristicHandle, uint8_t flags) = 0; | ||
| virtual CHIP_ERROR SetCharacteristicValue(uint8_t characteristicHandle, const ByteSpan & value) = 0; | ||
|
|
||
| // ----- Getters (matches existing BLEChannel) ----- |
There was a problem hiding this comment.
| // ----- Getters (matches existing BLEChannel) ----- |
| virtual CHIP_ERROR SetConnectionParams(const Optional<uint8_t> & connectionHandle, uint32_t intervalMin, uint32_t intervalMax, | ||
| uint16_t latency, uint16_t timeout) = 0; | ||
| virtual CHIP_ERROR CloseConnection(void) = 0; | ||
| virtual CHIP_ERROR SetAdvHandle(uint8_t handle) = 0; |
There was a problem hiding this comment.
Hum why a setAdvHandle? The handle is provided by the ble API. we don't know it before we create the adv set
| virtual CHIP_ERROR SetAdvertisingParams(uint32_t intervalMin, uint32_t intervalMax, uint16_t duration, | ||
| const Optional<uint16_t> & maxEvents, const Optional<uint8_t> & channelMap) = 0; | ||
| virtual CHIP_ERROR OpenConnection(const uint8_t * address, size_t addressLen, uint8_t addrType) = 0; | ||
| virtual CHIP_ERROR SetConnectionParams(const Optional<uint8_t> & connectionHandle, uint32_t intervalMin, uint32_t intervalMax, |
There was a problem hiding this comment.
why is connectionHandle optional and why pass it as reference? We only need the value
| * Connection handle type for BLE operations. Platform code casts to/from | ||
| * BLE_CONNECTION_OBJECT where required by the Matter BleLayer. | ||
| */ | ||
| using BleConnectionHandle = void *; |
There was a problem hiding this comment.
why a void pointer? I think we should infer the type from the implementing driver/channel.
For example, for efr32 this is plainly a uint8
| // ----- Address / identity ----- | ||
| virtual CHIP_ERROR GetAddress(uint8_t * addr, size_t * length) = 0; | ||
| virtual CHIP_ERROR GetDeviceName(char * buf, size_t bufSize) = 0; | ||
| virtual CHIP_ERROR SetDeviceName(const char * name) = 0; |
There was a problem hiding this comment.
maybe provide a length too
| // ----- Advertising ----- | ||
| virtual CHIP_ERROR StartAdvertising(const BleAdvertisingParams & params) = 0; | ||
| virtual CHIP_ERROR StopAdvertising() = 0; | ||
| virtual CHIP_ERROR SetAdvertisingData(const uint8_t * data, size_t length) = 0; |
There was a problem hiding this comment.
don't you have a AdvConfigStruct ? I see it use in the BleSideChannel.h
| import("//build_overrides/chip.gni") | ||
|
|
||
| # Matter BLE channel implementation for EFR32 (skeleton). | ||
| source_set("ble-efr32") { |
There was a problem hiding this comment.
I think we can combine the source_set ble-efr32
then source_set ble-siwx and source_set("ble") in the same build.gn
probably the src/platform/silabs/ble/BUILD.gn
| @@ -0,0 +1,129 @@ | |||
| /* | |||
There was a problem hiding this comment.
Do we need full re-implementation for the Matter channel for EFR32 and 917?
Don't we have some Common Functions/usage and then just a few API from the base classe that are override implementations in the EFR32 and 917 Matter channels?
Summary
This PR introduces the BLE channel abstraction and skeleton implementations so EFR32 and SiWx917 share a single API and both platforms build with the new structure.
Current logic:
There is a single BLEManagerImpl used for both EFR32 and SiWx917. The file is full of
#if SLI_SI91X_ENABLE_BLE / #elsebranches: one path uses EFR32 APIs (sl_bt_msg_t, sl_bt_, gatt_db.h), the other uses SiWx APIs (SilabsBleWrapper::BleEvent_t, rsi_ble_, BLE task, queue).Init, event handling (e.g. HandleConnectEvent, HandleWriteEvent), and BLE operations are all duplicated inside this one class, which makes the code hard to read and maintain.
What we’re doing:
We’re moving the platform-specific BLE logic out of BLEManagerImpl and into separate channel classes. BLEManagerImpl stays a single, shared implementation but no longer contains EFR/SiWx conditionals for the stack. Instead it will hold a
BleChannel*(e.g. BleChannelMatterEfr32 or BleChannelMatterSiWx), call a common API (e.g. channel->Init(), channel->ParseEvent(evt)), and get back a unified BleEvent via a callback. All sl_bt_* vs rsi_ble_* and event-type differences live inside the channel implementations, so we reduce the conditional logic in BLEManagerImpl and make adding or changing a platform a matter of implementing the channel interface instead of editing one big file.Changes:
src/platform/silabs/ble/BlePlatformTypes.h
Platform-agnostic types:
BleConnectionHandle, BleEventType, BleEvent, BleAdvertisingParams, BleConnectionParams, BleEventCallback. No stack-specific headers.
src/platform/silabs/ble/BleChannel.h
Abstract BLE channel interface:
Init, Shutdown; StartAdvertising, StopAdvertising, SetAdvertisingData; CloseConnection, UpdateConnectionParams; SendIndication, GetMTU; SetEventCallback, CanHandleEvent, ParseEvent(void* platformEvent); GetAddress, SetDeviceName; StartIndicationTimer, CancelIndicationTimer.
src/platform/silabs/ble/BleSideChannel.h
Abstract side-channel interface extending BleChannel:
ConfigureAdvertising(BleSideChannelAdvConfig), HandleReadRequest, HandleWriteRequest, HandleCCCDWriteRequest, UpdateMtu, AddConnection, RemoveConnection. For future provisioning/CLI side channel.
src/platform/silabs/ble/efr32/BleChannelMatterEfr32.{h,cpp}
Matter BLE channel skeleton for EFR32: implements all BleChannel methods with stubs (e.g. CHIP_NO_ERROR for init, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE or no-op where appropriate). No sl_bt_* calls yet.
src/platform/silabs/ble/SiWx/BleChannelMatterSiWx.{h,cpp}
Matter BLE channel skeleton for SiWx917: same as above for SiWx; no rsi_ble_* / SilabsBleWrapper usage yet.
src/platform/silabs/ble/BUILD.gn
Defines shared ble target (headers/types only, include_dirs for ble).
src/platform/silabs/ble/efr32/BUILD.gn
Defines ble-efr32 source set (compiles BleChannelMatterEfr32.cpp), depends on ble.
src/platform/silabs/ble/SiWx/BUILD.gn
Defines ble-siwx source set (compiles BleChannelMatterSiWx.cpp), depends on ble.
The complete refactor would be done in the upcoming phases:
Related issues
N/A
Testing
EFR32 and SiWx builds compile and link with the new skeleton.
No runtime BLE behavior change: BLEManagerImpl does not yet instantiate or use the new channels; that will be done in further phases.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines