feat(ffv)!: rename PowerMode → ListeningMode + terminology/HFP fixes (stacked on #177) - #529
Open
Ulrond wants to merge 1 commit into
Open
feat(ffv)!: rename PowerMode → ListeningMode + terminology/HFP fixes (stacked on #177)#529Ulrond wants to merge 1 commit into
Ulrond wants to merge 1 commit into
Conversation
Acts on the review feedback in PR #177: - Rename PowerMode → ListeningMode (ACTIVE_LISTEN / KEYWORD_ALERT / POWERED_OFF) to drop overlap with system power state vocabulary and describe the module's externally-observable listening behaviour instead. - Consistent terminology: "Keyword channel" → "Keyword Audio channel" (same for Continual Audio / Microphones Audio). - Pin a HAL-wide endian order on audio data — 16-bit signed little-endian PCM — so consumers no longer need per-platform byte-swap logic. - Surface supportedListeningModes in Capabilities so platforms can advertise which subset of modes they support. - Rewrite onKeywordDetected() and onEndOfCommand() docstrings. - Rework hfp-ffv.yaml: camelCase keys mirroring the AIDL field names, drop runtime-status fields (power_state, privacy_mode, hardware_failure) which belong in Status, not in HFP. Add supportedListeningModes + channelTypes. Mechanical changes only. Items from the review that need author judgement (removing fields with no consumer, defining HAL units for things currently described as vendor-defined, scoping the module's overview doc) are intentionally left for the author.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the FFV (Far Field Voice) HAL surface to use clearer “listening mode” terminology (renaming PowerMode → ListeningMode and related APIs), aligns channel terminology (“Keyword/Continual/Microphones Audio channel”), pins the documented PCM endianness, and refactors the FFV HFP YAML to mirror the Capabilities parcelable (including adding supportedListeningModes).
Changes:
- Rename
PowerModeenum/file/fields/methods/callbacks toListeningModeacross FFV AIDL and build wiring. - Extend
CapabilitieswithListeningMode[] supportedListeningModesand update HFP YAML shape/keys to matchCapabilities. - Documentation/terminology updates for channel naming and PCM encoding/endian wording.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ffv/current/hfp-ffv.yaml | Restructures FFV HFP to a capabilities: block mirroring Capabilities and adds supportedListeningModes / canonical channel types. |
| ffv/current/com/rdk/hal/farfieldvoice/Status.aidl | Renames powerMode field to listeningMode and updates related docs/imports. |
| ffv/current/com/rdk/hal/farfieldvoice/PowerMode.aidl | Removes the old PowerMode enum definition (replaced by ListeningMode). |
| ffv/current/com/rdk/hal/farfieldvoice/ListeningMode.aidl | Adds the new ListeningMode enum with updated terminology and mapping notes. |
| ffv/current/com/rdk/hal/farfieldvoice/IFarFieldVoiceEventListener.aidl | Renames callback to onEnteredListeningMode and updates parameter/docs. |
| ffv/current/com/rdk/hal/farfieldvoice/IFarFieldVoiceControllerListener.aidl | Clarifies wake-word / end-of-command callback docstrings and parameter descriptions. |
| ffv/current/com/rdk/hal/farfieldvoice/IFarFieldVoiceController.aidl | Renames setPowerMode → setListeningMode and updates channel docs (including endian/PCM wording). |
| ffv/current/com/rdk/hal/farfieldvoice/IFarFieldVoice.aidl | Updates top-level interface documentation to use Audio-channel terminology consistently. |
| ffv/current/com/rdk/hal/farfieldvoice/Capabilities.aidl | Adds supportedListeningModes and updates channel-type documentation language. |
| ffv/current/CMakeLists.txt | Updates the FFV AIDL source list to build ListeningMode.aidl instead of PowerMode.aidl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # (see ffv/current/com/rdk/hal/farfieldvoice/Capabilities.aidl) so a single | ||
| # grep finds both the API surface and the per-platform value. | ||
| # | ||
| # This is a reference HFP — SOC vendors derive platform-specific variants |
Comment on lines
+60
to
64
| * If channelType specifies a Continual Audio channel or Microphones Audio channel | ||
| * | ||
| * Audio data is written to the channel's pipe at the vendor/product specific sampling rate. The audio data | ||
| * format is vendor/product specific. The endian order of each sample value is that of the host processor's | ||
| * native endian order. | ||
| * Audio data is written to the channel's pipe at the channel's native sampling rate using the | ||
| * same 16-bit signed little-endian PCM encoding as the Keyword Audio channel. | ||
| * |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked review-feedback PR targeting #177's branch — applies the mechanical changes from my proposal comment. Merge into your branch (
feature/23-task-far-field-voice-hal) if it looks right; reject items individually if not.@lucienkennedylamb
What's in this PR
Renames —
PowerMode→ListeningMode, with values renamed to listening-capability vocabulary that doesn't overlap with system power state names:FULL_POWER→ACTIVE_LISTENSTANDBY→KEYWORD_ALERTDEEP_SLEEP→POWERED_OFFNONEkept as-isPowerMode.aidl→ListeningMode.aidl(git-mv preserves history)IFarFieldVoiceController.setPowerMode()→setListeningMode(),IFarFieldVoiceEventListener.onEnteredPowerMode()→onEnteredListeningMode(),Status.powerMode→Status.listeningMode,CMakeLists.txtsource list updated.Terminology consistency — every reference to a channel uses the "Audio" suffix so it's unambiguous:
Endian order pinned — replaced "host processor's native endian order" with "16-bit signed little-endian PCM" throughout the channel docs so consumers don't need per-platform byte-swap logic. (Pick a different value if 16-bit LE is wrong for the platform — the point is to define one, not to specifically pick LE.)
Capabilities extended — added
ListeningMode[] supportedListeningModesso platforms that only support a subset (e.g.KEYWORD_ALERT+POWERED_OFF, noACTIVE_LISTEN) can advertise that.Callback docstrings rewritten —
onKeywordDetected()andonEndOfCommand()got clearer descriptions including what zero offset means.HFP YAML reworked —
hfp-ffv.yaml:microphoneChannelCount, notmicrophone_channel_count)power_state,privacy_mode,hardware_failure,noise_reduction,beamforming,beamforming_type,noise_reduction_type— these belong inStatus/IFarFieldVoiceEventListenercallbacks, not in HFPsupportedListeningModesand the canonicalchannelTypesenumerationcapabilities:block that maps 1:1 with theCapabilitiesparcelableWhat's NOT in this PR (deliberately left for you)
These items from my proposal needed FFV-team judgement and I didn't want to guess:
docs/halif/ffv/current/far_field_voice.mdper the interface template — not done.FLUSHING/STARTING/STOPPING, you can trim. I didn't touch it.KeywordDetectInfoparcelable — this didn't exist in the current branch (already removed?), so the related comments in my proposal are moot. If it's coming back later, the unit/zero-meaning concerns from items 13-17 still apply.Test plan
PowerMode/setPowerMode/onEnteredPowerModenames (pre-1.0, but worth a grep across consumer repos)