Conversation
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
Co-authored-by: Ulrond <131959864+Ulrond@users.noreply.github.com>
Co-authored-by: Ulrond <131959864+Ulrond@users.noreply.github.com>
Co-authored-by: Ulrond <131959864+Ulrond@users.noreply.github.com>
…ibility) Co-authored-by: Ulrond <131959864+Ulrond@users.noreply.github.com>
Co-authored-by: Ulrond <131959864+Ulrond@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates from a shared common State.aidl to module-specific State.aidl enums, addressing the deprecation of the common State type and implementing the project's guideline for each module to define its own state management.
Key changes:
- Added new module-specific State.aidl files for videosink, videodecoder, hdmioutput, broadcast/frontend, avclock, and audiosink
- Updated import statements across all affected interfaces to reference module-specific State enums instead of the common one
- Added deprecation notice to the common State.aidl file
- Updated CMakeLists.txt files to include the new State.aidl files in builds
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| videosink/current/com/rdk/hal/videosink/State.aidl | New module-specific State enum for video sink component |
| videosink/current/com/rdk/hal/videosink/IVideoSinkEventListener.aidl | Updated import to use videosink-specific State |
| videosink/current/com/rdk/hal/videosink/IVideoSinkControllerListener.aidl | Updated import to use videosink-specific State |
| videosink/current/com/rdk/hal/videosink/IVideoSink.aidl | Updated import to use videosink-specific State |
| videosink/current/CMakeLists.txt | Added State.aidl to build sources |
| videodecoder/current/com/rdk/hal/videodecoder/State.aidl | New module-specific State enum for video decoder component |
| videodecoder/current/com/rdk/hal/videodecoder/IVideoDecoderEventListener.aidl | Updated import to use videodecoder-specific State |
| videodecoder/current/com/rdk/hal/videodecoder/IVideoDecoder.aidl | Updated import to use videodecoder-specific State |
| videodecoder/current/CMakeLists.txt | Added State.aidl to build sources and removed common State.aidl reference |
| hdmioutput/current/com/rdk/hal/hdmioutput/State.aidl | New module-specific State enum for HDMI output component |
| hdmioutput/current/com/rdk/hal/hdmioutput/IHDMIOutputEventListener.aidl | Updated import to use hdmioutput-specific State |
| hdmioutput/current/com/rdk/hal/hdmioutput/IHDMIOutput.aidl | Updated import to use hdmioutput-specific State |
| hdmioutput/current/CMakeLists.txt | Added State.aidl to build sources |
| hdmiinput/current/com/rdk/hal/hdmiinput/State.aidl | Updated package name from com.rdk.hal to com.rdk.hal.hdmiinput |
| common/current/com/rdk/hal/State.aidl | Added deprecation notice indicating modules should use their own State enums |
| broadcast/current/com/rdk/hal/broadcast/frontend/State.aidl | New module-specific State enum for broadcast frontend component |
| broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendListener.aidl | Updated import to use frontend-specific State |
| broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl | Updated import to use frontend-specific State |
| broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl | Updated import to use frontend-specific State |
| broadcast/current/CMakeLists.txt | Added State.aidl to build sources |
| avclock/current/com/rdk/hal/avclock/State.aidl | New module-specific State enum for AV clock component |
| avclock/current/com/rdk/hal/avclock/IAVClockEventListener.aidl | Updated import to use avclock-specific State |
| avclock/current/com/rdk/hal/avclock/IAVClockControllerListener.aidl | Updated import to use avclock-specific State |
| avclock/current/com/rdk/hal/avclock/IAVClock.aidl | Updated import to use avclock-specific State |
| avclock/current/CMakeLists.txt | Added State.aidl to build sources |
| audiosink/current/com/rdk/hal/audiosink/State.aidl | New module-specific State enum for audio sink component |
| audiosink/current/com/rdk/hal/audiosink/IAudioSinkEventListener.aidl | Updated import to use audiosink-specific State |
| audiosink/current/com/rdk/hal/audiosink/IAudioSinkControllerListener.aidl | Updated import to use audiosink-specific State |
| audiosink/current/com/rdk/hal/audiosink/IAudioSink.aidl | Updated import to use audiosink-specific State |
| audiosink/current/CMakeLists.txt | Added State.aidl to build sources |
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The AV Clock is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
This State enum is missing the FLUSHING state that exists in the deprecated common State.aidl (value 6). The common State.aidl includes: CLOSED = 1, OPENING = 2, READY = 3, STARTING = 4, STARTED = 5, FLUSHING = 6, STOPPING = 7, CLOSING = 8. This State enum jumps from STARTED = 5 directly to STOPPING = 7, omitting FLUSHING. If the FLUSHING state is intentionally removed for this module, the enum values after STARTED should be renumbered (STOPPING = 6, CLOSING = 7) to avoid gaps in the sequence.
| STOPPING = 7, | |
| /** | |
| * The AV Clock is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| STOPPING = 6, | |
| /** | |
| * The AV Clock is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 7 |
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The Audio Sink is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
This State enum is missing the FLUSHING state that exists in the deprecated common State.aidl (value 6). The common State.aidl includes: CLOSED = 1, OPENING = 2, READY = 3, STARTING = 4, STARTED = 5, FLUSHING = 6, STOPPING = 7, CLOSING = 8. This State enum jumps from STARTED = 5 directly to STOPPING = 7, omitting FLUSHING. If the FLUSHING state is intentionally removed for this module, the enum values after STARTED should be renumbered (STOPPING = 6, CLOSING = 7) to avoid gaps in the sequence.
| STOPPING = 7, | |
| /** | |
| * The Audio Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| STOPPING = 6, | |
| /** | |
| * The Audio Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 7 |
| enum State { | ||
| /** | ||
| * Initial state entered when the service connection is established. | ||
| */ | ||
| CLOSED = 1, |
There was a problem hiding this comment.
This State enum is missing the UNKNOWN state that exists in the deprecated common State.aidl (value 0). The common State.aidl includes UNKNOWN = 0 as the first state for uninitialized or unknown component states. All newly added State.aidl files start with CLOSED = 1, omitting UNKNOWN. This should be consistent across all modules - either all State enums should include UNKNOWN or none should. The audiodecoder State.aidl includes UNKNOWN = 0, suggesting it may be needed for proper state management.
| enum State { | ||
| /** | ||
| * Initial state entered when the service connection is established. | ||
| */ | ||
| CLOSED = 1, |
There was a problem hiding this comment.
This State enum is missing the UNKNOWN state that exists in the deprecated common State.aidl (value 0). The common State.aidl includes UNKNOWN = 0 as the first state for uninitialized or unknown component states. All newly added State.aidl files start with CLOSED = 1, omitting UNKNOWN. This should be consistent across all modules - either all State enums should include UNKNOWN or none should. The audiodecoder State.aidl includes UNKNOWN = 0, suggesting it may be needed for proper state management.
| enum State { | ||
| /** | ||
| * Initial state entered when the service connection is established. | ||
| */ | ||
| CLOSED = 1, |
There was a problem hiding this comment.
This State enum is missing the UNKNOWN state that exists in the deprecated common State.aidl (value 0). The common State.aidl includes UNKNOWN = 0 as the first state for uninitialized or unknown component states. All newly added State.aidl files start with CLOSED = 1, omitting UNKNOWN. This should be consistent across all modules - either all State enums should include UNKNOWN or none should. The audiodecoder State.aidl includes UNKNOWN = 0, suggesting it may be needed for proper state management.
| CLOSED = 1, | ||
|
|
||
| /** | ||
| * The Video Sink is transitioning from closed to ready state. | ||
| */ | ||
| OPENING = 2, | ||
|
|
||
| /** | ||
| * The Video Sink is open and ready to start, but in a stopped state. | ||
| */ | ||
| READY = 3, | ||
|
|
||
| /** | ||
| * The Video Sink is transitioning from ready to started state. | ||
| */ | ||
| STARTING = 4, | ||
|
|
||
| /** | ||
| * The opened Video Sink has been started. | ||
| */ | ||
| STARTED = 5, | ||
|
|
||
| /** | ||
| * The started Video Sink is stopping and flushing its internal state. | ||
| * Once flushed, the Video Sink enters the ready state. | ||
| */ | ||
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The Video Sink is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
The enum values in this new State.aidl are inconsistent with the existing hdmiinput State.aidl. The hdmiinput State.aidl starts with CLOSED = 0, while all the newly added State.aidl files start with CLOSED = 1. This inconsistency needs to be resolved to maintain a uniform state machine model across all modules. Either update hdmiinput to align with the new pattern (CLOSED = 1, OPENING = 2, etc.) or update all new State.aidl files to match hdmiinput's pattern (CLOSED = 0, OPENING = 1, etc.).
| CLOSED = 1, | |
| /** | |
| * The Video Sink is transitioning from closed to ready state. | |
| */ | |
| OPENING = 2, | |
| /** | |
| * The Video Sink is open and ready to start, but in a stopped state. | |
| */ | |
| READY = 3, | |
| /** | |
| * The Video Sink is transitioning from ready to started state. | |
| */ | |
| STARTING = 4, | |
| /** | |
| * The opened Video Sink has been started. | |
| */ | |
| STARTED = 5, | |
| /** | |
| * The started Video Sink is stopping and flushing its internal state. | |
| * Once flushed, the Video Sink enters the ready state. | |
| */ | |
| STOPPING = 7, | |
| /** | |
| * The Video Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| CLOSED = 0, | |
| /** | |
| * The Video Sink is transitioning from closed to ready state. | |
| */ | |
| OPENING = 1, | |
| /** | |
| * The Video Sink is open and ready to start, but in a stopped state. | |
| */ | |
| READY = 2, | |
| /** | |
| * The Video Sink is transitioning from ready to started state. | |
| */ | |
| STARTING = 3, | |
| /** | |
| * The opened Video Sink has been started. | |
| */ | |
| STARTED = 4, | |
| /** | |
| * The started Video Sink is stopping and flushing its internal state. | |
| * Once flushed, the Video Sink enters the ready state. | |
| */ | |
| STOPPING = 5, | |
| /** | |
| * The Video Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 6 |
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The Video Sink is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
This State enum is missing the FLUSHING state that exists in the deprecated common State.aidl (value 6). The common State.aidl includes: CLOSED = 1, OPENING = 2, READY = 3, STARTING = 4, STARTED = 5, FLUSHING = 6, STOPPING = 7, CLOSING = 8. This State enum jumps from STARTED = 5 directly to STOPPING = 7, omitting FLUSHING. If the FLUSHING state is intentionally removed for this module, the enum values after STARTED should be renumbered (STOPPING = 6, CLOSING = 7) to avoid gaps in the sequence.
| STOPPING = 7, | |
| /** | |
| * The Video Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| STOPPING = 6, | |
| /** | |
| * The Video Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 7 |
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The Frontend is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
This State enum is missing the FLUSHING state that exists in the deprecated common State.aidl (value 6). The common State.aidl includes: CLOSED = 1, OPENING = 2, READY = 3, STARTING = 4, STARTED = 5, FLUSHING = 6, STOPPING = 7, CLOSING = 8. This State enum jumps from STARTED = 5 directly to STOPPING = 7, omitting FLUSHING. If the FLUSHING state is intentionally removed for this module, the enum values after STARTED should be renumbered (STOPPING = 6, CLOSING = 7) to avoid gaps in the sequence.
| STOPPING = 7, | |
| /** | |
| * The Frontend is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| STOPPING = 6, | |
| /** | |
| * The Frontend is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 7 |
| enum State { | ||
| /** | ||
| * Initial state entered when the service connection is established. | ||
| */ | ||
| CLOSED = 1, |
There was a problem hiding this comment.
This State enum is missing the UNKNOWN state that exists in the deprecated common State.aidl (value 0). The common State.aidl includes UNKNOWN = 0 as the first state for uninitialized or unknown component states. All newly added State.aidl files start with CLOSED = 1, omitting UNKNOWN. This should be consistent across all modules - either all State enums should include UNKNOWN or none should. The audiodecoder State.aidl includes UNKNOWN = 0, suggesting it may be needed for proper state management.
| /** | ||
| * Initial state entered when the service connection is established. | ||
| */ | ||
| CLOSED = 1, |
There was a problem hiding this comment.
The enum values in this new State.aidl are inconsistent with the existing hdmiinput State.aidl. The hdmiinput State.aidl starts with CLOSED = 0, while this file starts with CLOSED = 1. This inconsistency needs to be resolved to maintain a uniform state machine model across all modules. Either update hdmiinput to align with this pattern or update this file to match hdmiinput's pattern.
| CLOSED = 1, | |
| CLOSED = 0, |
Ulrond
force-pushed
the
feature/24-task-common-hal---review
branch
from
2f0cef7 to
e1b2abf
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| /** | ||
| * The started AV Clock is stopping and flushing its internal state. | ||
| * Once flushed, the AV Clock enters the ready state. |
There was a problem hiding this comment.
The STOPPING state is assigned value 7, but the previous state STARTED has value 5. This creates a gap where value 6 is unused. Consider either using consecutive values (6 for STOPPING) or documenting why value 6 is intentionally skipped.
| * Once flushed, the AV Clock enters the ready state. | |
| * Once flushed, the AV Clock enters the ready state. | |
| * | |
| * NOTE: Numeric value 6 is intentionally left unused to preserve | |
| * backwards compatibility with legacy implementations that defined | |
| * an intermediate state between STARTED and STOPPING. |
| STOPPING = 7, | ||
|
|
||
| /** | ||
| * The Audio Sink is transitioning from ready to closed state. | ||
| */ | ||
| CLOSING = 8 |
There was a problem hiding this comment.
The STOPPING state is assigned value 7, but the previous state STARTED has value 5. This creates a gap where value 6 is unused. Consider either using consecutive values (6 for STOPPING) or documenting why value 6 is intentionally skipped.
| STOPPING = 7, | |
| /** | |
| * The Audio Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 8 | |
| STOPPING = 6, | |
| /** | |
| * The Audio Sink is transitioning from ready to closed state. | |
| */ | |
| CLOSING = 7 |
| * The started Video Sink is stopping and flushing its internal state. | ||
| * Once flushed, the Video Sink enters the ready state. | ||
| */ | ||
| STOPPING = 7, |
There was a problem hiding this comment.
The STOPPING state is assigned value 7, but the previous state STARTED has value 5. This creates a gap where value 6 is unused. Consider either using consecutive values (6 for STOPPING) or documenting why value 6 is intentionally skipped.
| * The started Video Decoder is stopping and flushing its internal state. | ||
| * Once flushed, the Video Decoder enters the ready state. | ||
| */ | ||
| STOPPING = 7, |
There was a problem hiding this comment.
The STOPPING state is assigned value 7, but the previous state STARTED has value 5. This creates a gap where value 6 is unused. Consider either using consecutive values (6 for STOPPING) or documenting why value 6 is intentionally skipped.
| STOPPING = 7, | |
| STOPPING = 6, |
|
|
||
| /** | ||
| * The started Frontend is stopping and flushing its internal state. | ||
| * Once flushed, the Frontend enters the ready state. |
There was a problem hiding this comment.
The STOPPING state is assigned value 7, but the previous state STARTED has value 5. This creates a gap where value 6 is unused. Consider either using consecutive values (6 for STOPPING) or documenting why value 6 is intentionally skipped.
| * Once flushed, the Frontend enters the ready state. | |
| * Once flushed, the Frontend enters the ready state. | |
| * | |
| * Note: Integer value 6 is intentionally reserved (unused) to maintain | |
| * compatibility with existing implementations, so STOPPING uses value 7. |
github-project-automation
bot
moved this from Review Requested
to Approved For Merge
in halif_aidl
Review all usage of state.aidl across the interface