-
Notifications
You must be signed in to change notification settings - Fork 0
PR: Feature/23 task far field voice hal #177
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
base: develop
Are you sure you want to change the base?
Conversation
Update FFV feature branch with latest from Develop
Ulrond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did an initial review with amba on the ffv hals
| * limitations under the License. | ||
| */ | ||
| package com.rdk.hal.farfieldvoice; | ||
| import com.rdk.hal.State; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need this, the new model is that each module will have it's own State, and you have that anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be removed.
| * Keyword channel audio processing is initialized and keyword detection is started. Upon keyword | ||
| * detection, audio samples are written to the Keyword channel's pipe. | ||
| * | ||
| * Audio data is signed 16 bits per sample at 16kHz sampling rate. The endian order of each sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is hardcoded. would it be for the future? Would we not need to expand this into a getCaps and then we're flexible as the system expands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The middleware and VREX only support 16 bit 16kHz audio and is not configurable. There is no use case for the HAL to support any other sample size or sampling rate.
| * number with respect to the audio samples written to the Keyword channel's pipe. A sample offset value | ||
| * of zero corresponds to the first sample written after opening the Keyword channel. | ||
| * | ||
| * The Keyword channel can be opened only in Standby or Full Power modes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On documentation and cross-references
When we mention types from other files, let’s reference them explicitly so Doxygen can link them (e.g., PowerMode::FULL_POWER, PowerMode::STANDBY). This helps readers jump to the definitions.
On power modes and this module’s responsibility
The system is responsible for control of power, it's not clear why this module needs to know. What behaviour changes are expected in your power modes this it's clear from the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update types with explicit names.
Far Field Voice in Sky Glass uses an external DSP in order to support low power requirements while in Standby state. The external DSP is listening for a key word while the SOC is sleeping. Upon key word detect, the DSP signals the SOC to wake up. The FFV HAL needs to load DSP firmware and communicate key word language information every time the SOC transitions between Full Power and Standby states. Future products may not use an external DSP but may have a lower power state while in Standby. Thus, the FFV HAL needs to know in which state it should operate. For future products that don't need to operate differently based on power state, the FFV HAL can do nothing and respond with success.
| * If successful, creates and returns a pipe for passing raw microphone data to the client and | ||
| * the Microphones channel is in the open state. | ||
| * | ||
| * Audio data is signed 32 bits per sample at 16kHz sampling rate. The endian order of each sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to move this to caps, and this should be static, that's just what the current system does, this API is required to be future proof.
| interface IFarFieldVoiceController { | ||
|
|
||
| /** | ||
| * Open the Keyword channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not clear from the documentation (maybe because we don't have a higher level description doc), on what a keyword channel is and what it does.
| @VintfStability | ||
| @Backing(type="int") | ||
|
|
||
| enum PowerMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use words that don't get mixed up with the system power states, that means real things for the module..
Your describing Wakefullness / alert status with these ones enum VoiceModuleState
FULLPOWER -> ACTIVE_LISTEN
STANDBY -> KEYWORD_ALERT
DEEP_SLEEP -> POWERED_OFF
Should we be listing enum WakeFullnessMode
FULLPOWER -> LIVE_STREAMING
STANDBY -> WAKE_DETECT
DEEP_SLEEP -> INACTIVE
Alternative enum ListeningCapability
FULLPOWER -> VOICE_READY
STANDBY -> SILENT_GUARD
DEEP_SLEP -> DORMANT
You would need to list supported modes in the HFP, so that each platform could support some if not all.
so the HFP would look like
iFarFieldVoice:
# far_field_voice component HFP content
microphone_channel_count: 1 # channels
# Defines the static operational modes supported by the hardware.
# Values are non-system power state names: ACTIVE_LISTEN, KEYWORD_ALERT, POWERED_OFF
supported_states:
- ACTIVE_LISTEN
- KEYWORD_ALERT
- POWERED_OFF
# Detailed configuration and types
channel_types: ["KEYWORD", "VOICE_COMMAND"] # Array of supported channel types
| * This is for information only and useful to log when the Far Field | ||
| * Voice service is opened. | ||
| */ | ||
| @utf8InCpp String codeBuildVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here? and where is it expected to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is informational to the FFV HAL team to know which FFV HAL build was used when doing debug and analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the lower SubSystemVersion, since it may not be a code build but a firmware or software binary, why does the MW need to know this?
| * Total number of keyword channel samples lost due to buffer overflow. | ||
| * | ||
| * This is for information only and useful to log at the end of a session. | ||
| * Non zero values may indicate inadequate CPU time or buffer space and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indicate inadequate CPU time > how would you know this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FFV HAL will detect that audio data buffers are overflowing indicating the middleware is unable to keep up with real time audio.
| * | ||
| * This is for information only and useful to log if an error condition occurs. | ||
| */ | ||
| long vendorErrorCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are you expecting to log this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make sense for the middleware to write this to a log for debug and analysis when failures occur in the field.
| /** | ||
| * Indicates if the Continual channel is supported. | ||
| */ | ||
| bool continualChannelSupported; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there caps missing? You've got more in your hfp
hal:
components:
far_field_voice:
caps:
microphone_channel_count: 1 # channels
privacy_mode: false -> supports_privacy_mode?
hardware_failure: false -> Not required for the hfp
noise_reduction: true # Indicates noise reduction support -> Caps?
beamforming: true # Indicates beamforming support
beamforming_type: "fixed" # Optional: fixed, adaptive, etc.
noise_reduction_type: "spectral" # Optional: spectral, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will investigate. Noise reduction settings should not be visible to the middleware.
|
|
Ulrond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments added
| * | ||
| * @param[in] failureCode The reason for the failure. | ||
| */ | ||
| void onHardwareFailed(in FailureCode failureCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to understand the usecase for this and what actions if any do the upper layers take, does it need to pull the interfaces and bring it back up or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The appropriate response may be vendor or product dependent. The intent is to provide a way for the FFV HAL to inform the middleware that it is non functional because of a hardware related fault (e.g. failure to communicate with an external DSP or microphone input failure). At least one retry to restart the HAL may be appropriate. At the very least, the middleware should log the occurrence for later analysis.
For review by HAL team