feat(gfps): Added processing to control non-discoverable advertising … #466
Conversation
|
✅Static analysis result - no issues found! ✅ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Hey @hirokada-san, Thanks for this PR! I don't think we want to have this be a kconfig-setting and we definitely can't have espp::gfps_service be dependent on an opaque app.hpp and bb::App API.
Instead what I'd suggest is that the gfps_service take in another function that the user can provide which would then be called in the nearby_platform_SetRemotePasskey implementation, if the function is not nullptr. I'd expect the function to be provided in the espp::gfps::Config struct, similar to the notify_callback_t that's provided today. For backwards compatibility for the API, I'd expect the function to default to nullptr in the struct. If no callback is provided, then the original implementation of the SetRemotePasskey function runs (which ends up calling injectConfirmPasskey on the first connected peer). If the callback is provided, then it should be the one that handles all the work, and should simply be provided the passkey from the outer call.
I believe that such a design requires no Kconfig setting, no app.hpp or other opaque API dependencies, and maximizes flexibility for use of the API in the future. Additionally, it continues to remain backwards compatible because existing callers (who are not providing the function pointer today) will have the same behavior as they do today.
|
@finger563 -san, thank you for your review and feedback. |
There was a problem hiding this comment.
@hirokada-san, Thanks for the updates, this is definitely better.
There are still some improvements I'd like to see, and I'm not sure your code is fully operational - there are some callbacks you use and function implementations you define that I cannot find references to, so I believe they will not actually do anything.
|
@finger563 -san, thank you so much for the code review. |
| typedef std::function<void(uint64_t, const uint8_t *key)> account_key_write_callback_t; | ||
|
|
||
| /// Callback triggered when the GFPS layer finishes constructing a Non-Discoverable Advertisement payload | ||
| /// @param adv_data Pointer to raw advertisement payload bytes | ||
| /// @param len Length of the advertisement data | ||
| typedef std::function<void(const uint8_t *adv_data, size_t len)> nda_ready_callback_t; |
There was a problem hiding this comment.
For these two functions, can we change the use of const uint8_t* and const uint8_t*, size_t to be const std::span<uint8_t>&. Given that the first function's argument is known size of 16 bytes, we can use
const std::span<uint8_t, 16>&While for the second, we'd still use
const std::span<uint8_t>&This provides a lightweight accessor which enables the application code to use range-based methods and other accessors, while also including OOB checking and other type/memory safety helpers.
Then in the event handler which calls these callbacks you can simply call:
g_account_key_write_cb(payload->peer_address, std::span<uint8_t, 16>(payload->key_data, 16));and
g_non_discoverable_advertisement_cb(std::span<uint8_t>(payload->adv_data, payload->length));
There was a problem hiding this comment.
@hirokada There are still a few lingering issues - one is relatively minor (the use of the new typedef), but the other breaks the build (the use of non-existent event enums). Does this require a fork of the google/nearby library to work or where are these events coming from?
I've also suggested an API update to use std::span where possible to do a lightweight type-conversion when passing data out of the low-level gfps library to app c++ code.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the GFPS service layer by exposing Account Key write and NDA-ready events through opt-in application callbacks, enabling tighter integration during subsequent pairing flows.
- Added two new callbacks (
on_account_key_write_callback,on_non_discoverable_advertisement_ready_callback) in the GFPSConfigand hooked them into the embedded Fast Pair client. - Updated
init/deinitworkflows innearby_ble.cppto register, invoke, and clear these callbacks safely. - Refactored
GfpsService::initoverloads to accept and merge a user-providedConfig, while preserving the internal notification mechanism.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| components/gfps_service/src/nearby_ble.cpp | Registered C-compatible handlers for account key writes and NDA payload readiness; stored and cleared callbacks in init/deinit; updated nearby_fp_client_Init. |
| components/gfps_service/include/gfps_service.hpp | Added overloads of init to accept custom Config; merged application callbacks with internal notify logic. |
| components/gfps_service/include/gfps.hpp | Introduced new callback typedefs for passkey injection, account key write, and NDA readiness. |
| components/gfps_service/detail/nearby | Submodule bump to deliver embedded client callback support. |
…for subsequent pairing from the application layer.
…ling - Added optional callback to notify app when NDA advertisement payload is prepared - Added callback to notify when Account Key is written - Passkey response destination changed from fixed (latest peer) to actual sender - Behavior customizable via espp::gfps::Config (all new callbacks default to nullptr) - Preserves backward compatibility and introduces no app-layer dependencies
- Enables app-layer control of Non-Discoverable Advertisement dispatch - Refines callback struct lifetime handling via static allocation - Consolidates lambda dispatch logic for cleaner event handling - Applies typedef aliases for callback clarity in Config - Incorporates reviewer feedback for safer and clearer design
…lback integration
…d for const-correctness
hirokada
force-pushed
the
feature/add-app-control-subpair-flow
branch
from
ffa3375 to
ffddb56
Compare
|
@finger563 -san, I’ve applied the suggestions from Copilot and confirmed that the modified source behaves correctly. |
@hirokada -san, thank you for all the work you put into reviewing and testing this! |
GFPS: Expose AccountKey write and NDA generation events for subsequent pairing integration
Description
Summary
This PR introduces internal event exposure mechanisms across the GFPS service layer and the embedded Fast Pair library, enabling application-level control during the subsequent pairing phase.
To support external advertisement orchestration, the GFPS layer now receives structured notifications when:
These enhancements enable tight integration with existing application-layer advertisement logic, especially in scenarios where discoverable and non-discoverable flows must be coordinated.
This PR builds upon esp-cpp/nearby#2, now integrated via submodule update.
Changes Overview
GFPS Layer (Service-level integration for application control)
Files:
gfps.hppgfps_service.hppnearby_ble.cppEnhancements:
Configcallbacks:account_key_write_callback_tnda_ready_callback_texternhooks for event propagationgfps::deinit()to prevent residual behaviorEmbedded Layer (GFPS client callbacks)
Files:
nearby_fp_client.hnearby_fp_client.cEnhancements:
NearbyFpClientCallbacks:on_account_key_writtenon_nondiscoverable_advertisement_readyon_event()dispatchnearby_fp_client_Deinit()stub for lifecycle managementCompatibility
Motivation and Context
Prior implementations assumed advertising behavior was controlled solely within the GFPS library. However, in real-world applications—especially during subsequent pairing—advertisements must align with runtime platform logic.
By exposing these internal events:
Prior GFPS implementations routed passkey confirmation to the last-connected peer, regardless of pairing initiator. This change ensures the passkey is returned to the correct device that initiated the pairing exchange.
How has this been tested?
Verified correct emission of
NdaPayloadReadypost-generationConfirmed
AccountKeyWrittenis triggered only after key registration completesTested suppression and injection of NDA ads using
ConfigcallbacksValidated fallback behavior (no callbacks set) matches legacy logic
The following actual devices were used for these checks.
Pixel 7 Pro (Android 16)
Galaxy S24 (Android 15)
Also verified legacy behavior when callbacks are unset, ensuring non-regression.
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.