Feature/ #253 Extend the Broadcast HAL to cover DVB-TCS and ATSC1.0

[japsen73]

No description provided.

[Copilot]

Pull Request Overview

This pull request enhances the broadcast HAL API with satellite (DVB-S) and LNB control support, adds new signal information properties, and improves API consistency.

• Adds DVB-S standard support with new enumerations (DvbSStandard, RollOff, SignalDetectMode)
• Introduces LNB (Low-Noise Block) controller interface with voltage and tone control
• Refactors demux architecture to use a provider/controller pattern with improved resource management

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TuneStatus.aidl, TransmissionMode.aidl, Modulation.aidl, etc.	Added @Backing(type="int") annotation for enum stability
SignalInfoValue.aidl	Added dvbTStandard and dvbSStandard union members
SignalInfoProperty.aidl	Added DVB_T_STANDARD and DVB_S_STANDARD properties, fixed typos in comments
SignalDetectMode.aidl, RollOff.aidl	New enumerations for signal detection modes and roll-off values
LnbVoltage.aidl, LnbTone.aidl	New enumerations for LNB control parameters
ILnbController.aidl	New interface for controlling satellite LNB equipment
IFrontend.aidl	Added methods for ID retrieval, open status check, data provider acquisition, and LNB control
IDemuxDataProvider.aidl, IDemuxController.aidl	New interfaces for demux provider/controller pattern
IDemux.aidl	Refactored from direct filter management to connect/disconnect pattern with capabilities
DemuxCapabilities.aidl	New parcelable for demux capability reporting
IBroadcastManager.aidl	Added version methods and getDemux/getDemuxIds methods
DvbSStandard.aidl	New enumeration for DVB-S/S2/S2X standards
Version.aidl, ImplementationVersion.aidl	New parcelables for version reporting
CMakeLists.txt	Added new files to build, fixed typos and formatting

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Ulrond]

@japsen73 any action on this one.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.

[Copilot]

🟡 Changes recommended

Critical build-install failures and Binder/AIDL compatibility issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (43)

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:49

• This replaces the existing broadcast-manager method set/order with new version, ID-list, and lookup methods. Since AIDL transaction IDs are positional, existing generated clients will call incompatible transactions rather than the methods they were built for. Preserve the existing API and append compatible methods, or publish this manager redesign under a new interface version.

    ImplementationVersion getImplementationVersion();

    /**
     * Gets the platform list of frontend IDs.
     *

broadcast/current/com/rdk/hal/broadcast/ImplementationVersion.aidl:45

• The article is incorrect in this public interface documentation; use an implementation (and the standard compound adjective implementation-specific).

     * This is a implementation specific string, that has no semantics attached to it. The only requirements are that it

broadcast/current/com/rdk/hal/broadcast/demux/FilterParameters.aidl:21

• FilterParameters is used by IDemuxController.openFilter() and contains demux filter parameter unions, but its class documentation describes tuner tuning parameters. This misleading description can cause consumers to misunderstand the purpose of the union; document it as demux filter-specific parameters.

/**
 * Tuner-specific tuning parameters.

broadcast/current/com/rdk/hal/broadcast/demux/FilterType.aidl:38

• Renaming/replacing the existing MPEG2TS enum value with MPEG2TS_DATA reuses its backing value for a different public symbol and removes the old name. Existing clients may send the old value, so retain the old member/value and append new filter types, or version the API.

    /** Mpeg2 transport stream data packets */
    MPEG2TS_DATA,

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:72

• The failure contract is contradictory: an already-connected demux is documented to throw EX_ILLEGAL_STATE, but the nullable return is also documented as null for that same condition. Choose one error mechanism and make the nullability/exception documentation consistent so clients know whether to inspect the return value or catch the Binder exception.

     * @exception ::android::binder::Status::EX_ILLEGAL_ARGUMENT The DemuxDataProvider is already connected to another
     *                                                           Demux.
     * @exception ::android::binder::Status::EX_ILLEGAL_STATE isConnected() is true.
     *
     * @param[in] provider The DemuxDataProvider to connect the Demux to.
     *
     * @returns IDemuxController or null on failure (e.g. already connected).
     */
    @nullable IDemuxController connect(in IDemuxDataProvider provider);

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:49

• getId() and isConnected() are inserted ahead of the demux operations and the old source/filter lifecycle is replaced by different methods. Because AIDL transaction IDs are positional, existing clients cannot interoperate with this current interface after these changes. Retain the old method order/signatures and append additions, or move the redesign to a new interface version.

    /** Get the ID of this demux. */
    Id getId();

    /**
     * Check whether this demux is already connected.

broadcast/current/com/rdk/hal/broadcast/demux/IMpeg2TsDataFilter.aidl:56

• ananogously is a spelling error in the filter behaviour documentation; use analogously.

     * though, ananogously to setPids(). One practical use-case for this is if the list of PIDs becomes larger than

broadcast/current/com/rdk/hal/broadcast/frontend/FrontendCapabilities.aidl:59

• Adding atsc before the existing specific-capability alternatives shifts the union tag for every prior frontend type. Existing Binder clients will therefore decode a DVB-C/S/T capability as the wrong alternative. Preserve the existing order and append ATSC, or version the parcelable/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:32
• This changes the backing values and removes previously exposed frontend types, so the same integer sent over Binder is now decoded as a different type (and old enum values become unavailable). A current AIDL interface must retain existing enum values and append new ones; keep the old members or introduce a versioned replacement.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:51
• getId() and isOpen() are inserted before the existing frontend methods, while open()/close() later use a different lifecycle and signature. AIDL transaction IDs are positional, so old clients will invoke the wrong transactions against this interface. Preserve the existing method order/signatures and append compatible methods, or expose this redesign under a new interface version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:27
• uninterferred is a spelling error in this public interface documentation.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:30
• The possessive pronoun is misspelled in this public API documentation.
  broadcast/current/com/rdk/hal/broadcast/frontend/ILnbController.aidl:24
• LNB controllers are obtained through IFrontend.openLnb(), but the cross-reference points to IFrontendController, which contains no LNB lifecycle details. Link to IFrontend so the public documentation leads clients to the actual factory method.
  broadcast/current/com/rdk/hal/broadcast/frontend/Modulation.aidl:38
• Adding atsc before the existing modulation alternatives changes the discriminator of every existing DVB modulation. Existing Binder clients will decode the same union tag as a different modulation type. Preserve the prior order and append ATSC, or version the union/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:37
• FRONTEND_TYPE is inserted before the existing signal-property members, which changes the backing integer of every following property. For example, existing clients expecting DEMOD_LOCK at its previous value will decode the new value as a different property. Preserve the existing numeric assignments and append additions, or publish this as a new interface version.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoValue.aidl:47
• Adding this union member at the front shifts the ordinal/tag of every existing SignalInfoValue member, and the reworked member names also prevent old generated clients from matching the same union alternatives. Binder union alternatives are wire data, so preserve the existing order and names and append new alternatives, or version the interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneParameters.aidl:35
• Adding atsc at the front of this union changes the discriminator of the existing DVB tune-parameter alternatives. Existing clients and services will no longer agree on the type carried over Binder. Append new alternatives after the existing members or version the union/interface.
  broadcast/current/docs/broadcast.md:63
• This documentation says functions do not return booleans, but the public API adds IFrontend.isOpen() and IDemux.isConnected() as boolean methods. Rewrite this principle to describe the actual rule, otherwise clients may treat those state-query methods as inconsistent or deprecated.
  broadcast/current/docs/broadcast.md:32
• it's is the contraction of it is; the possessive form required here is its.
  broadcast/current/docs/broadcast.md:53
• The capability type is FrontendCapabilities, and its fields are minFrequency/maxFrequency; there is no frontend.Capabilities.minFrequencyHz or maxFrequencyHz in the AIDL. This guidance points implementers to nonexistent symbols.
  broadcast/current/interface.yaml:7
• build_modules.sh only recognises a top-level imports: at two spaces and its entries at four spaces (build_modules.sh:357-364). With this four/eight-space indentation, ringbuffer@current is omitted from the dependency graph, so manifest builds can schedule Broadcast before ringbuffer. Please use the repository's two/four-space indentation.
  broadcast/current/interface.yaml:2
• These new current interfaces are not listed in either root build manifest. build_modules.sh manifest --file versions_current.yaml and the smoke test derive the expected component set from */current/interface.yaml, so broadcast and ringbuffer will be omitted from manifest builds; add both components at current to versions_current.yaml and versions_released.yaml.
  broadcast/metadata.yaml:45
• acquission is a spelling error in the metadata scope; use acquisition so the published component description is correct.
  format_aidl.sh:8
• The formatting script only traverses broadcast, so none of the new ringbuffer AIDL files are formatted when this script is run. Add the new module to the directory list so the repository-wide formatter covers all AIDL introduced by this PR.
  ringbuffer/current/CMakeLists.txt:110
• ringbuffer declares no HAL imports and does not link any dependency library, but this unconditional check makes the documented standalone configure fail unless an unrelated HALIF_LIB_DIR is supplied. Make the dependency link-directory setup optional, as this component has no HAL dependencies.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:33
• This public documentation refers to a waitForData method, but both interfaces expose acquire() instead. Consumers following the description will look for an operation that does not exist; update the reference to acquire.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:40
• This second occurrence also names the nonexistent waitForData operation. The seek offsets are supplied by acquire() for both producer and consumer sides.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The repository's Doxygen guideline requires @returns for return-value documentation, not @return. Use the project-standard tag here so generated API documentation is consistent.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:104
• The repository's Doxygen guideline requires @returns for return-value documentation, not @return. Use the project-standard tag here so generated API documentation is consistent.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:48
• IRingBuffer is never returned by any of the new AIDL interfaces and has no service-name constant, so a client has no defined way to obtain an object on which to call setSize() or register a producer/consumer. Expose it through a factory/registered service (and define its service name), or keep this implementation-facing contract out of the public AIDL module.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The repository's Doxygen guideline requires @returns for return-value documentation, not @return. Use the project-standard tag here so generated API documentation is consistent.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:64
• When the requested size exceeds the ring buffer capacity, this says acquire() returns that full size, but RingBufferAcquireResult exposes only one offset into the fixed-size mapping and cannot describe a contiguous region larger than the buffer. A producer following this contract could write past the mapped ring buffer. Define and enforce a maximum request (or specify a safe wrapping/chunking protocol) instead of promising the unbounded requested size.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSinkListener.aidl:55
• No operation on IRingBufferSource or IRingBuffer requests a flush, so this callback has no possible trigger in the published API. Either expose the consumer flush operation and define its ordering, or remove this callback before the interface is frozen.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:67
• RingBufferAcquireResult has no overflow member; overflow is represented by RingBufferErrorCode/the listener callbacks. This documentation directs clients to consume a field that cannot be generated or read, so remove the nonexistent-field reference.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:74
• The repository's Doxygen guideline requires @returns for return-value documentation, not @return; the article before RingBufferAcquireResult is also incorrect. Use the project-standard wording here.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSourceListener.aidl:45
• This callback is for the consumer, and the surrounding documentation consistently describes readable data, but this parameter says writing. Consumers may interpret the callback value in the wrong direction; document it as bytes available for reading.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• typesafe is misspelled/grammatically incorrect here. Use the established compound adjective type-safe and the correct article.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:37
• acquiry is not the operation name used by this API and is a spelling error in the error-code documentation; use acquire result to match the acquire() methods and RingBufferAcquireResult type.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• overlow is a spelling error in this public field description.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:22
• The article is incorrect before IRingBuffer (pronounced as an initial vowel sound) in this public type description.
  ringbuffer/current/mkdocs.yml:1
• The root mkdocs.yml includes broadcast/current/mkdocs.yml but has no include for this new component, so the ringbuffer documentation will not be published or reachable from the site navigation. Add the ringbuffer configuration to the root navigation.
  ringbuffer/metadata.yaml:27
• generate_rag_report.sh only classifies metadata types equal to SOC or OEM (scripts/generate_rag_report.sh:179-191). SOC/OEM is therefore an unrecognised value and will be omitted from the corresponding report; choose one supported type or update the metadata schema/tooling to support a third value.
  ringbuffer/metadata.yaml:62
• The reviewer key is misspelled as Broadcsast_Team, but the canonical mapping is Broadcast_Team (scripts/team_mapping.yaml:10). As written, this reviewed status will not map to the Broadcast team.

• Files reviewed: 101/101 changed files
• Comments generated: 8
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved critical build integration and Binder API compatibility issues make this high-risk for automated approval.

Review details

Suppressed comments (38)

broadcast/current/CMakeLists.txt:136

• This standalone target installs to lib/halif, while build_modules.sh and the other standalone module CMake files use lib/rdk-halif-aidl as the SDK library directory. A direct install will place this library outside the directory consumed by the repository's build and packaging flow. Use the standard configurable install destination with a lib/rdk-halif-aidl default.

include(GNUInstallDirs)
install(TARGETS ${LIB_NAME}
    LIBRARY DESTINATION lib/halif
            PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
                        GROUP_READ GROUP_EXECUTE
                        WORLD_READ WORLD_EXECUTE)

broadcast/current/CMakeLists.txt:129

• This target adds SDK/dependency link directories but does not disable CMake's build RPATH. The generated .so can therefore contain absolute build-machine paths, which is rejected by Yocto package QA and is not usable as a portable deployed library. Apply the same SKIP_BUILD_RPATH/BUILD_WITH_INSTALL_RPATH/INSTALL_RPATH handling used by audiodecoder/current/CMakeLists.txt:133-148.

target_link_libraries(${LIB_NAME} PRIVATE binder utils ringbuffer-vcurrent-cpp)

set_target_properties(${LIB_NAME} PROPERTIES OUTPUT_NAME "${LIB_NAME}")

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:52

• This new method is inserted before the existing frontend methods, which changes every subsequent Binder transaction code; the same interface also removes the previous demux-opening API. Existing AIDL clients will therefore dispatch to the wrong methods or lose an API. Preserve the existing method order and signatures, append additions at the end, or introduce a new version/interface for the incompatible redesign.

    ImplementationVersion getImplementationVersion();

    /**
     * Gets the platform list of frontend IDs.
     *
     * @returns Array of IFrontend.Id values for all frontends on this platform.
     */
    IFrontend.Id[] getFrontendIds();

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:98

• The declaration returns @nullable IDemuxSoftwareInput.Id, but the @returns text promises an IDemuxSoftwareInput interface. This can mislead clients about whether they can call methods directly or must acquire the ID first. Document the actual return type.

     * @returns IDemuxSoftwareInput or null on failure (e.g. maximum number of DemuxSoftwareInput instances has been
     *          reached).
     */
    @nullable IDemuxSoftwareInput.Id createSoftwareInput();

broadcast/current/com/rdk/hal/broadcast/demux/IMpeg2TsDataFilter.aidl:56

• ananogously is misspelled in the public filter documentation; correct it to analogously.

     * though, ananogously to setPids(). One practical use-case for this is if the list of PIDs becomes larger than

broadcast/current/com/rdk/hal/broadcast/frontend/DvbTCapabilities.aidl:49

• DvbTCodeRates is a parcelable containing the coupled lowPriority and highPriority values, but this field exposes only Value[] and loses that pairing. A client cannot determine which HP/LP combinations the tuner accepts; expose supported combinations as DvbTCodeRates[], or explicitly define independent-rate semantics.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTHierarchy.aidl:13
• This type is DvbTHierarchy, but its file-level description says MISO modes. The description is swapped with DvbTMiso.aidl and misleads users of the DVB-T tuning API.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTMiso.aidl:14
• The enum values are spatial-stream modes (SISO, MISO), not hierarchical transmission modes. This description misleads clients about what the capability controls; describe it as DVB-T MISO modes.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:33
• The existing frontend enum values were removed and the remaining values now receive different implicit ordinals. AIDL enum ordinals are part of the serialized contract, so old clients and servers can interpret the same value differently. Retain every existing value and ordinal, appending new values only, or use a new enum/interface version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:73
• These methods are inserted ahead of the existing frontend operations, while the old listener/state operations and the old open contract were removed or changed. That changes the Binder transaction table and breaks clients compiled against the existing interface. Keep the old methods in their original order and append compatible additions, or publish a new interface/version for this lifecycle redesign.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:51
• The controller's existing close operation has been removed and the remaining operations were reordered so tune becomes transaction 0. That is a Binder ABI break independent of the manager/frontend changes: an older client calling close will invoke a different operation on this interface. Preserve the old method sequence and append new operations, or expose this redesign under a new interface/version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:28
• uninterferred is misspelled in the controller contract documentation; correct it to uninterrupted.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:83
• Capabilities is no longer the type exposed by this API; the method returns data from FrontendCapabilities. This parameter documentation therefore names a type clients cannot find.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:53
• This enum both inserts new members before existing ones and renames existing members such as RFLEVEL/UNC. That changes serialized ordinals and source/API names, so clients using the previous signal-property contract will not interoperate. Preserve the old members and ordinals and append new properties, or introduce a new version.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoValue.aidl:63
• This union changes existing signal members' names/types (for example lock booleans become enum values and RF level changes type) instead of preserving the prior wire layout. AIDL union discriminants and payload types are serialized over Binder, so old clients can decode a different member or type. Preserve the existing members/order and add a compatible extension, or publish this as a new interface/version.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneParameters.aidl:38
• The union member names/order no longer preserve the existing TuneParameters wire layout, while dvbS is inserted in the middle. Union member ordinals are serialized over Binder, so existing clients can send a value that the new server decodes as a different member. Keep the old members in order and append the new member, or version the union/interface.
  broadcast/current/docs/broadcast.md:32
• By it's nature is grammatically incorrect in this public documentation; use the possessive its.
  broadcast/current/docs/broadcast.md:55
• The example references frontend.Capabilities.minFrequencyHz/maxFrequencyHz, but the AIDL exposes frontend.FrontendCapabilities.minFrequency/maxFrequency. Readers following this requirement will look up fields that do not exist. Update the example to the current type and field names.
  broadcast/current/interface.yaml:7
• build_modules.sh parses imports: only at two-space indentation and dependency entries at four spaces (see build_modules.sh:357-364). With the four/eight-space indentation here, ringbuffer is omitted from the dependency graph, so a manifest build can compile broadcast before its ringbuffer library and fail at link time. Match the repository's manifest indentation or make the parser YAML-aware.
  broadcast/current/interface.yaml:7
• The new broadcast and ringbuffer components are absent from versions_current.yaml, which is the manifest used for the in-development cohort. Consequently, ./build_modules.sh manifest --file versions_current.yaml silently omits this feature and never builds either module; add both components to the manifest.
  broadcast/metadata.yaml:45
• acquission is misspelled in the component scope metadata, so the published description contains an incorrect term.
  format_aidl.sh:14
• Because clang-format is the left side of an && list, its failure is ignored for that iteration; if a later file formats successfully, the while pipeline exits successfully and this script reports success while leaving the failed file unchanged. Check the formatter result explicitly and abort/clean up the temporary file on failure.
  ringbuffer/current/CMakeLists.txt:113
• ringbuffer has no HAL dependencies and links only binder and utils, so requiring HALIF_LIB_DIR makes its standalone configure fail unless callers provide an unrelated variable. Remove this dependency check and the corresponding target_link_directories block; keep only the Binder SDK inputs.
  ringbuffer/current/CMakeLists.txt:117
• The ringbuffer standalone target also adds the Binder SDK link directory without disabling build RPATH, so its build artifact can embed an absolute SDK path. This violates the portable-library/Yocto packaging behaviour used by the other module CMake files. Add the same configurable no-RPATH target-property block used by audiodecoder/current/CMakeLists.txt:133-148.
  ringbuffer/current/CMakeLists.txt:124
• This standalone target installs to lib/halif, but the repository's build and packaging flow consumes out/target/lib/rdk-halif-aidl (build_modules.sh:739-741; see the existing module CMake defaults). Installing ringbuffer here therefore puts the library outside the expected SDK directory. Use the standard configurable install destination with a lib/rdk-halif-aidl default.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:33
• IRingBufferSink and IRingBufferSource expose acquire(), not waitForData(). This public documentation therefore points implementers to a nonexistent API and makes the shared-memory access contract ambiguous. Replace this reference with the actual acquire operations (and update the repeated reference in the following paragraph).
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:41
• This second description also refers to the nonexistent waitForData method, so consumers following the documented file-descriptor semantics cannot map it to the actual API. Use acquire here as well.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The repository's Doxygen convention uses @returns, not @return; rename this tag so the generated API documentation recognises it.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:104
• The repository's Doxygen convention uses @returns, not @return; rename this tag so the generated API documentation recognises it.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The repository's Doxygen convention uses @returns, not @return; this tag is rendered inconsistently by the documentation tooling. Rename the tag to @returns.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:67
• The overflow contract says discontinuity will be reported through RingBufferAcquireResult.overflow, but that parcelable has no overflow member. Consumers therefore have no way to detect the condition as documented. Add a defined result field and serialization semantics, or change this documentation to describe the actual OVERFLOW error/callback behaviour.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:74
• The repository's Doxygen convention uses @returns, not @return; rename this tag so the generated API documentation recognises it.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSourceListener.aidl:45
• This callback is for readable data, but its parameter is documented as bytes available for writing. That reverses the meaning of the value presented to consumers and conflicts with the threshold description above. Document it as bytes available for reading.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• The public documentation says An typesafe ID; the article and compound adjective should be corrected for generated API documentation.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:37
• acquiry is not the intended term in this public error-code documentation; use acquire/acquisition consistently with the API method name.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• overlow is a spelling error in the public parcelable documentation and does not match the setOverflowing/isOverflowing API names.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:22
• The article is incorrect in this public type description; use an IRingBuffer.
  ringbuffer/metadata.yaml:62
• The reviewer key is misspelled as Broadcsast_Team, while scripts/team_mapping.yaml defines Broadcast_Team. Review automation cannot map this entry to the broadcast reviewer team, so the recorded sign-off can be ignored.

• Files reviewed: 101/101 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved API compatibility, build integration, and contract issues require changes and human review.

Review details

Suppressed comments (37)

broadcast/current/CMakeLists.txt:133

• The standalone install destination is lib/halif, while the repository's build and Yocto consumers expect HAL libraries under lib/rdk-halif-aidl. A direct install of Broadcast therefore puts the library outside the directory downstream consumers search.

    LIBRARY DESTINATION lib/halif

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:45

• Adding getImplementationVersion() before the existing methods changes every subsequent Binder transaction code. An older client will send the code for getFrontendIds() and the new service will decode it as getImplementationVersion(), so the current interface is not wire-compatible. Append new methods after the existing declarations or introduce a versioned interface instead.

    ImplementationVersion getImplementationVersion();

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:69

• The manager no longer exposes the previous demux-opening operation and instead only enumerates and retrieves IDs. This removes functionality from the existing manager contract for clients that need to create/open a demux; retain the old operation or publish this redesign under a new version.

    IDemux.Id[] getDemuxIds();

    /**
     * Get the demux interface for the given ID.
     *
     * @exception ::android::binder::Status::EX_ILLEGAL_ARGUMENT if the ID is invalid.
     */
    IDemux getDemux(in IDemux.Id demuxId);

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:70

• The contract is internally ambiguous: the exceptions above say connect() must throw when the provider or demux is already connected, while this return documentation lists “already connected” as a case that returns null. Clients cannot implement reliable error handling until this is made one consistent mechanism.

     * @returns IDemuxController or null on failure (e.g. already connected).

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:49

• The demux interface similarly replaces the existing source/filter lifecycle with a different set of methods. Existing clients will continue to use the old transaction numbers, which now map to different operations or no longer exist. Preserve the current declarations and append compatible additions, or publish this redesign as a new interface/version.

    /** Get the ID of this demux. */
    Id getId();

    /**
     * Check whether this demux is already connected.

broadcast/current/com/rdk/hal/broadcast/demux/IMpeg2TsDataFilter.aidl:56

• Correct the spelling in this analogy reference.

     * though, ananogously to setPids(). One practical use-case for this is if the list of PIDs becomes larger than

broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:73

• This int field uses an enum member (ConstantInt.INVALID_PLS_CODE) as its default. The AIDL default expression is not an int, so generation fails type checking; use a typed integer constant or the literal -1.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:33
• This enum no longer preserves the existing backing values: entries such as ANALOG, ATSC3, ISDB_S, ISDB_T, DTMB, and IPTV were removed, so retained values such as ATSC and DVB_C now receive different implicit integers. Existing clients serialising those values will be decoded as different frontend types. Preserve the old values explicitly and append new values, or version the interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:50
• This replaces the existing IFrontend method contract, including the prior lifecycle/listener/state operations, rather than extending it. Removing or reordering methods changes Binder transaction codes and breaks clients compiled against the current interface. Keep the old methods in their original order and append additions, or expose this redesign under a new interface/version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:27
• Use the standard word “uninterrupted” here.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:41
• FRONTEND_TYPE is inserted before the existing properties while all later members remain implicitly numbered. That shifts the serialized integer for every existing property (for example, DEMOD_LOCK), so old clients and new services will request/return different signal properties. Keep the existing ordinals explicit and append newly introduced properties.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:49
• These renamed property members also remove the old source-level names and can change their implicit numeric positions. This breaks clients compiled against the previous signal-info contract; retain aliases/ordinals or expose the redesign under a new version.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoValue.aidl:51
• The union's existing members have been renamed/retyped and the discriminator order has changed. A client compiled against the previous SignalInfoValue will use the old union tag and decode a different type or field, which is a Binder API break; preserve the old union layout or publish this as a new version.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneParameters.aidl:38
• The union declaration reorders the existing dvbC/dvbT members and inserts dvbS between them. AIDL union tags are declaration-order based, so an old peer can deserialize a dvbT or dvbC payload as the wrong type. Preserve the existing member order and append the new member, or define a versioned union.
  broadcast/current/docs/broadcast.md:52
• The example refers to frontend.Capabilities.minFrequencyHz and maxFrequencyHz, but the AIDL type is FrontendCapabilities with fields named minFrequency and maxFrequency. Following this guidance leads clients to nonexistent API members.
  broadcast/current/docs/broadcast.md:63
• This design rule is contradicted by the new API: ILnbController.isOverloaded() returns boolean. The current wording tells clients that boolean-returning functions do not exist, so it should describe the common operation/error pattern without prohibiting state-returning methods.
  broadcast/current/docs/broadcast.md:32
• Use the possessive pronoun without an apostrophe.
  broadcast/current/interface.yaml:7
• build_modules.sh parses imports with the exact ^ imports: / ^ - indentation (see build_modules.sh:357-364). With four/eight spaces here, ringbuffer is omitted from the dependency graph, so manifest builds can configure Broadcast before its headers and library are available. Use the repository's two-space YAML indentation for this block.
  broadcast/metadata.yaml:45
• The metadata uses the misspelling acquission; use acquisition so the component scope is machine-reviewable and readable.
  broadcast/metadata.yaml:24
• The broadcast AIDL changes remove/rename members, change signatures and reorder union fields, which the repository's versioning guide classifies as a breaking surface change (docs/standards/versioning-guide.md:135-167). The metadata still declares 0.1.0.0; either preserve compatibility or bump the major component version before release.
  format_aidl.sh:9
• The formatter added by this PR only traverses broadcast, so none of the new ringbuffer AIDL files are formatted when the repository's formatting script is run. Add the ringbuffer directory to the list of formatted roots.
  ringbuffer/current/CMakeLists.txt:110
• Ringbuffer has no HAL imports and links only binder and utils, but this standalone CMake unconditionally fails unless the caller supplies an unused HALIF_LIB_DIR. That prevents the documented single-module build from working with only the Binder SDK; make this dependency-directory setup conditional, as in bootreason/current/CMakeLists.txt:111-113.
  ringbuffer/current/CMakeLists.txt:121
• The standalone install destination is lib/halif, but this repository's build and Yocto consumers stage HAL libraries under lib/rdk-halif-aidl (for example, build_modules.sh and the other current CMakeLists). Installing this module here leaves the new library outside the directory downstream consumers search.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:32
• waitForData is referenced here, but neither ring-buffer endpoint declares that method; the API exposes acquire() instead. This leaves the public documentation describing a callable operation that does not exist.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The new public documentation uses @return, while this repository's AIDL documentation standard requires @returns. Update the tag so generated API documentation handles this return description consistently.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The new public documentation uses @return, while this repository's AIDL documentation standard requires @returns. Update the tag so generated API documentation handles this return description consistently.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:62
• Use the repository's UK spelling “behaviour” in this new public interface documentation.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:64
• The overflow contract says acquire() returns the full requested size immediately, but a request larger than the configured ring-buffer size cannot be represented by the documented contiguous offset/bytes result without writing past the mapped buffer. Define a maximum-size check/cap (and its error or partial-result behavior) before exposing this API.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:66
• The documentation promises an overflow flag on RingBufferAcquireResult, but that parcelable only defines id, offset, bytes, and remaining. A consumer therefore has no way to observe the promised discontinuity indicator. Add the field at the end of the parcelable or revise this contract to specify how overflow is reported.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:74
• The new public documentation uses @return, while this repository's AIDL documentation standard requires @returns. Update the tag so generated API documentation handles this return description consistently.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSourceListener.aidl:45
• This callback is for readable data, but its parameter is documented as bytes available for writing. Consumers implementing the callback from this description can interpret the value in the wrong direction.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• The new comment has a grammatical error (An typesafe ID). Use the standard hyphenated adjective and article.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:31
• The repository uses UK English in interface documentation; use “initialised” here (and consistently in the new broadcast enum comments) rather than the US spelling.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:37
• The error-code documentation uses the nonstandard misspelling acquiry; use acquisition to describe the result that has not yet been released.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• Correct the typo in the overflow description.
  ringbuffer/current/mkdocs.yml:2
• Adding a module-local MkDocs configuration is not enough to publish these new ringbuffer docs: the root mkdocs.yml has no ringbuffer entry in its navigation, so this page will be omitted from the generated documentation site. Add the module to the root navigation alongside the other HAL modules.
  ringbuffer/metadata.yaml:62
• Correct the typo in the metadata reviewer key.

• Files reviewed: 101/101 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved AIDL compatibility, generation, and build-integration issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (41)

Previously missed (6) — in code that hasn't changed since the last review.

broadcast/current/CMakeLists.txt:129

• This target also adds absolute Binder/dependency directories with target_link_directories but never disables the build RPATH, so the installed broadcast .so can contain absolute build paths and fail Yocto package QA. Match the RPATH handling used by the other current-module CMake files.
  ringbuffer/current/CMakeLists.txt:117
• Because this target adds absolute Binder/dependency directories with target_link_directories, the current CMake properties cause those build paths to be emitted as RPATH/RUNPATH in the shipped .so. This fails the same Yocto packaging check addressed in avclock/current/CMakeLists.txt:133-148; apply the no-build-RPATH properties here as well.
  broadcast/current/docs/broadcast.md:63
• This new API declares boolean query methods (isOpen, isConnected, and isOverloaded), so the blanket statement that functions do not return boolean is false and conflicts with the interfaces documented by this page. Narrow the rule to mutating operations or document the query-method exception.
  format_aidl.sh:9
• The formatter only visits broadcast, so the newly added ringbuffer AIDL files are excluded from the repository's formatting check. Include the ringbuffer directory in FORMAT_DIRS so both components are formatted consistently.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:67
• This text claims overflow is reported through RingBufferAcquireResult.overflow, but that parcelable has no overflow field; it only contains id, offset, bytes, and remaining. Document the existing IRingBufferSourceListener.onError/RingBufferErrorCode.OVERFLOW path instead, or add the missing field to the API.
  ringbuffer/current/mkdocs.yml:1
• This adds a ringbuffer documentation site, but the root mkdocs.yml has no !include ringbuffer/current/mkdocs.yml entry. Consequently these pages are not reachable from the generated documentation navigation; add the component to the root navigation.

broadcast/current/CMakeLists.txt:133

• This target is installed under lib/halif, but the repository's staging/Yocto contract and dependent CMake files use lib/rdk-halif-aidl. The broadcast library will therefore be placed outside the standard package/staging location; use the standard configurable install directory.

    LIBRARY DESTINATION lib/halif

broadcast/current/CMakeLists.txt:31

• The standalone invocation example points HALIF_LIB_DIR at lib/halif, but this repository stages HAL libraries under lib/rdk-halif-aidl. Following this command leaves the ringbuffer dependency undiscoverable; update the example (and matching diagnostics below) to the standard path.

#         -DHALIF_LIB_DIR=/path/to/out/target/lib/halif \

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:45

• getImplementationVersion() is inserted before the existing getFrontendIds() method. AIDL transaction IDs are positional, so this shifts every following method and makes existing clients and servers dispatch different calls. Preserve the existing order and append additions at the end, or introduce a new versioned interface.

    ImplementationVersion getImplementationVersion();

broadcast/current/com/rdk/hal/broadcast/ImplementationVersion.aidl:45

• The new public documentation uses the ungrammatical phrase a implementation specific; correct the article and compound adjective.

     * This is a implementation specific string, that has no semantics attached to it. The only requirements are that it

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:49

• getId() and isConnected() are inserted before the existing demux methods, and the old demux lifecycle/filter contract is replaced below. This changes positional AIDL transaction IDs and removes operations needed by existing clients. Keep the old interface intact and append compatible additions, or create a new versioned interface.

    /** Get the ID of this demux. */
    Id getId();

    /**
     * Check whether this demux is already connected.

broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:67

• ConstantInt.INVALID_INPUT_STREAM_ID is an enum value assigned as the default of an int field, rather than an integer constant. This is rejected by the AIDL type checker; use an int constant or the literal -1.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTTuneParameters.aidl:75
• ConstantInt is an enum, so ConstantInt.INVALID_PLP_ID is enum-typed while this field is int; AIDL's parcelable default type must match the field and this prevents interface generation. Use an integer constant/literal (or redefine the shared constants as const int) here.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:33
• Existing enum members are removed/reordered here without explicit numeric assignments. Because AIDL encodes enum values by their ordinal, old values will be interpreted as different frontend types by this version. Preserve every existing member with its original numeric value and append new values, or use a new version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:50
• These new methods are inserted before the existing frontend operations, while this interface also changes/removes lifecycle and listener methods below. That is not a backward-compatible AIDL evolution: old transaction IDs and method signatures must remain unchanged. Append new methods after the existing contract or publish a new interface version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:45
• The controller interface now starts with tune() and no longer preserves the previous controller operations/listener contract. Removing or moving existing AIDL methods changes transaction IDs and breaks old clients; retain the old methods in their original order and append additions, or use a new versioned interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:41
• This enum's existing values are reordered and renamed without explicit numeric assignments. That changes the wire value associated with existing signal properties, so old clients can request one property and receive another. Retain the original names/ordinals and append new properties, or publish a new version.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoValue.aidl:49
• The union members have been renamed/removed and their types changed in this update. Union alternatives are identified by their serialized tag, so changing this layout makes values produced by an older client decode as a different type or fail. Preserve the old alternatives and tags and append new ones, or introduce a new parcelable version.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneParameters.aidl:34
• The tuning union's alternatives and types have been changed in place. Stable AIDL unions use serialized alternative tags, so this is incompatible with clients using the previous TuneParameters layout. Keep the existing alternatives/tags and append compatible alternatives, or define a new versioned union.
  broadcast/current/docs/broadcast.md:52
• The capability type and field names in this rule no longer exist. The API exposes FrontendCapabilities.minFrequency and maxFrequency, so clients following this documentation will look up invalid members.
  broadcast/current/docs/broadcast.md:32
• The phrase uses the possessive contraction it's where the pronoun its is required.
  broadcast/current/interface.yaml:7
• The supported manifest builds enumerate only components listed in versions_current.yaml/versions_released.yaml; neither broadcast nor its new ringbuffer dependency is listed, so build_modules.sh manifest silently omits this entire feature. Add the new components to the development manifest (and define the intended released-cohort behaviour) or update manifest discovery before relying on this import.
  broadcast/current/interface.yaml:7
• The new Broadcast interface imports ringbuffer and adds frontend, demux, and CA functionality, but the feature profile still declares only interfaceVersion. This leaves the machine-readable HFP unable to describe the capabilities introduced by this PR, contrary to the repository's HFP contract; add the corresponding static capability declarations or explicitly define why this profile is intentionally empty.
  broadcast/metadata.yaml:45
• acquission is a spelling error in the component scope description; use acquisition so the metadata is machine-readable and professionally documented.
  broadcast/metadata.yaml:44
• The metadata says tuning and state changes are delivered to registered listeners, but the updated frontend API contains no tuning/state listener registration or callback interface. This makes the component scope inaccurate; remove the listener claim or add the corresponding API.
  format_aidl.sh:14
• A failed clang-format is the first command in an && list inside the loop, so set -e does not necessarily terminate that iteration; if a later file succeeds, the loop can finish successfully while leaving the failed file unformatted. Run the formatter and mv as separate commands (or explicitly propagate the failure).
  ringbuffer/current/CMakeLists.txt:109
• Ringbuffer has no HAL dependency and links only binder and utils, but this block makes every standalone build fail unless an unrelated HALIF_LIB_DIR is supplied. Keep this search path optional, as the no-dependency module CMake files do.
  ringbuffer/current/CMakeLists.txt:121
• The standalone target is installed under lib/halif, while the repository's staging/Yocto contract and dependent CMake files use lib/rdk-halif-aidl. A ringbuffer built and staged by this rule will not be found through HALIF_LIB_DIR, so broadcast cannot link against it; use the standard configurable install directory.
  ringbuffer/current/CMakeLists.txt:31
• The standalone invocation example points HALIF_LIB_DIR at lib/halif, but this repository stages HAL libraries under lib/rdk-halif-aidl. Following this command leaves the dependency undiscoverable; update the example (and matching diagnostics below) to the standard path.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:32
• waitForData is not part of either interface; both sides expose acquire(). This stale method name makes the shared-memory access contract point clients to an API they cannot call.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The repository documentation rule requires @returns, but this new method uses the deprecated @return tag. The generated API documentation will therefore be inconsistent with the project standard.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:104
• The repository documentation rule requires @returns, but this new method uses the deprecated @return tag. Replace the tag to keep generated API documentation consistent with the rest of the interface.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The repository documentation rule requires @returns, but this new method uses @return; replace the tag and the incorrect article in the acquire contract.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:64
• In an overflowing ring buffer, reaching the physical end of the contiguous region can still make RingBufferAcquireResult.bytes smaller than the requested amount. The acquire-result contract explicitly permits that, so clients must not assume this call always returns the requested byte count.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:74
• The repository documentation rule requires @returns, but this new method uses @return; replace the tag and the incorrect article in the acquire contract.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSourceListener.aidl:45
• This callback belongs to the consumer/source side, so the parameter description should report bytes available for reading. As written it tells implementers and generated documentation the opposite direction.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• The public type documentation has a grammar/wording error: the article before typesafe is incorrect and the standard term is type-safe.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:37
• acquiry is not the term used by the surrounding API (acquire/acquired); this typo makes the overflow condition harder to understand in the error contract.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• overlow is a typo in the public parcelable field documentation; it should say overflow so generated API documentation describes the setting correctly.
  ringbuffer/current/docs/ringbuffer.md:24
• Unlike the existing HAL documentation, this new public interface has an empty requirement row and placeholder sections for lifecycle, memory, buffer ownership, and mapping semantics. Those details are essential to implement the shared ring-buffer contract, so the component is not sufficiently specified for clients or vendors to interoperate.
  ringbuffer/metadata.yaml:62
• The reviewer key is misspelled as Broadcsast_Team, so it does not match the Broadcast_Team owner/team key used above and will not be recognised by metadata consumers.

• Files reviewed: 101/101 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate AIDL, API-compatibility, build, manifest, and documentation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (22)

broadcast/current/CMakeLists.txt:136

• This installs the library under lib/halif, but the repository's standalone CMakeLists and build_modules.sh contract use lib/rdk-halif-aidl (for example, avclock/current/CMakeLists.txt:156 and build_modules.sh:655). Downstream packaging and dependency resolution will not find this artifact at the documented location; use the common, overridable install directory.

    LIBRARY DESTINATION lib/halif
            PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
                        GROUP_READ GROUP_EXECUTE
                        WORLD_READ WORLD_EXECUTE)

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:45

• getImplementationVersion() is inserted before the existing methods, so every following AIDL transaction code changes. A client built against the previous current interface will dispatch getFrontendIds() as this new method and the remaining calls to the wrong transactions; preserve the existing order and append additions, or publish a new interface version.

    ImplementationVersion getImplementationVersion();

broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:32

• Removing the old enum members renumbers the values on the wire: the previous ATSC value was 2, but it is now 1, while old DVB_C/DVB_S/DVB_T values also shift. Existing clients and services will therefore decode the same value as a different frontend type; retain the existing members/assignments and append new values, or version the API.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:64
• This changes the existing getCapabilities() return type from nullable Capabilities to non-null FrontendCapabilities, which is a wire/API-breaking signature change; the old open/close contract is also replaced rather than extended. Keep the existing interface contract and append compatible methods, or move this redesign to a new version/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:37
• Inserting FRONTEND_TYPE and ACTUAL_FREQUENCY before the existing properties shifts every existing enum value (for example, old DEMOD_LOCK = 1 becomes 3); renaming/removing RFLEVEL and UNC is also source incompatible. Existing clients will request or decode the wrong property, so preserve the old numeric assignments and names or version the interface.
  broadcast/current/docs/broadcast.md:32
• The possessive is incorrect here; the sentence needs the contraction-free possessive form.
  broadcast/current/docs/broadcast.md:53
• This example still refers to the removed frontend.Capabilities type and its old minFrequencyHz/maxFrequencyHz fields, while the API now exposes FrontendCapabilities.minFrequency and maxFrequency. The published guidance will lead clients to nonexistent symbols.
  broadcast/current/interface.yaml:1
• These new interface.yaml files are not represented in either version manifest. The smoke test counts every */current/interface.yaml but manifest mode builds only explicit entries, so the current cohort will expect 23 libraries while building 21, and the released-cohort coverage check also fails. Add broadcast and ringbuffer to the manifests, pinning them to current until snapshots exist.
  broadcast/metadata.yaml:45
• The scope entry contains a spelling error.
  format_aidl.sh:9
• The new ringbuffer AIDL files are not included in FORMAT_DIRS, so running this formatter leaves the new module out while formatting broadcast. Add the new module directory to keep the formatting script covering all AIDL introduced by this change.
  ringbuffer/current/CMakeLists.txt:109
• ringbuffer/current/interface.yaml declares no imports and this target links only Binder, but configuration still aborts unless HALIF_LIB_DIR is supplied. A dependency-free ringbuffer standalone build therefore fails unless callers provide an unused variable; make this dependency path optional or remove it.
  ringbuffer/current/CMakeLists.txt:124
• This installs the library under lib/halif, while the repository's standalone CMakeLists and build_modules.sh contract use lib/rdk-halif-aidl (for example, avclock/current/CMakeLists.txt:156 and build_modules.sh:655). Downstream packaging and dependency resolution will not find this artifact at the documented location; use the common, overridable install directory.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:33
• Both references in this description name waitForData, but neither ring-buffer side defines such a method; the available operation is acquire. This makes the core memory-access contract point clients to a nonexistent API.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The project documentation standard uses @returns for return-value documentation; this new AIDL comment uses the prohibited @return tag.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:104
• The project documentation standard uses @returns for return-value documentation; this new AIDL comment uses the prohibited @return tag.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:48
• IRingBuffer is the module's main interface but does not declare the required serviceName constant. Without it, implementations have no interface-defined Binder registration name; add the module's service name, following the main-interface pattern used throughout the repository.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The project documentation standard uses @returns for return-value documentation; this new AIDL comment uses the prohibited @return tag.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSinkListener.aidl:55
• There is no consumer-facing method on IRingBufferSource or IRingBuffer that requests a flush, so this callback cannot be triggered through the public API. Add an explicit flush-request operation (and define its lifecycle) or remove this callback and its documentation.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSourceListener.aidl:45
• The source callback reports readable data, but this parameter is documented as bytes available for writing. That description can cause a consumer to use the notification with the wrong meaning.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• This comment is grammatically incorrect: use “A type-safe ID” rather than “An typesafe ID”.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• overlow is misspelled in this public field description.
  ringbuffer/metadata.yaml:62
• The reviewer team name is misspelled, so metadata consumers will see a different stakeholder key from the declared Broadcast_Team owner.

• Files reviewed: 101/101 changed files
• Comments generated: 6
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved API compatibility, build integration, and AIDL contract issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (36)

broadcast/current/CMakeLists.txt:136

• This standalone target installs to lib/halif, but the repository staging contract and downstream build scripts use out/target/lib/rdk-halif-aidl (see cmake_stage_modules.cmake:52 and avbuffer/current/CMakeLists.txt:157-162). A successful install here therefore leaves the library outside the SDK path consumed by dependants. Use the standard HALIF_INSTALL_LIBDIR default and update the lib/halif examples in this file as well.

install(TARGETS ${LIB_NAME}
    LIBRARY DESTINATION lib/halif
            PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
                        GROUP_READ GROUP_EXECUTE
                        WORLD_READ WORLD_EXECUTE)

broadcast/current/CMakeLists.txt:129

• The target links through ${BINDER_SDK_DIR} and ${HALIF_LIB_DIR} but leaves CMake's build RPATH enabled. The Yocto recipe copies the build-tree .so directly, so this embeds absolute SDK/staging paths in the shipped broadcast library and can fail package QA; clear the build/install RPATH as done in avclock/current/CMakeLists.txt:133-148.

set_target_properties(${LIB_NAME} PROPERTIES OUTPUT_NAME "${LIB_NAME}")

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:59

• This changes an existing VINTF interface: getFrontend is now non-nullable and the prior demux-opening operation has been removed. Existing generated clients and implementations cannot remain source/API compatible with that contract. Preserve the old declarations and append additions, or introduce a new version/interface for the redesign.

    IFrontend getFrontend(in IFrontend.Id frontendId);

broadcast/current/com/rdk/hal/broadcast/ImplementationVersion.aidl:45

• The article should be an before implementation-specific.

     * This is a implementation specific string, that has no semantics attached to it. The only requirements are that it

broadcast/current/com/rdk/hal/broadcast/demux/IDemuxSoftwareInput.aidl:80

• The normal lifecycle is acquireDataProvider() → IDemux.connect() → IDemux.disconnect() → releaseDataProvider(). At release time the software input is no longer connected, so this exception description is inverted and makes the documented cleanup sequence appear invalid.

     * @exception ::android::binder::Status::EX_ILLEGAL_STATE The software input is not connected to a Demux.

broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:67

• ConstantInt is an AIDL enum, but these fields are declared as int; an enum member is not an integer literal/const int for a parcelable default and will not type-check as an int default. Use an integer default (or define actual integer constants) instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:73
• ConstantInt is an AIDL enum, but this field is declared as int; an enum member is not an integer literal/const int for a parcelable default and will not type-check as an int default. Use an integer default (or define an actual integer constant) instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTTuneParameters.aidl:75
• ConstantInt is an AIDL enum, but this field is declared as int; an enum member is not an integer literal/const int for a parcelable default and will not type-check as an int default. Use an integer default (or define an actual integer constant) instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:32
• The updated enum no longer contains the previous frontend-type values. Removing or renumbering enum members changes the wire values seen by existing clients and can make previously valid values unknown. Preserve the existing members/ordinals and append new types instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:100
• The normal lifecycle is acquireDataProvider() → IDemux.connect() → IDemux.disconnect() → releaseDataProvider(). At release time the frontend is no longer connected, so this exception description is inverted and makes the documented cleanup sequence appear invalid.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:73
• This replaces the existing open(listener) contract with open() and drops the frontend state/listener notification path. That is a breaking change to an existing VINTF interface, not a backward-compatible extension. Retain the old method or publish a new interface version.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:87
• Changing close() to require an IFrontendController parameter changes the existing AIDL method signature and transaction contract. Existing clients cannot call this updated interface. Keep the old method and append a new operation, or version the interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:45
• The controller contract removes existing lifecycle/state/listener operations and starts with a different method set. Removing methods from a VINTF-stable AIDL interface breaks generated clients and servers; retain the old methods and append additions, or create a new interface/version.
  broadcast/current/com/rdk/hal/broadcast/frontend/TransmissionMode.aidl:30
• This changes the existing TransmissionMode type from an enum to a union while keeping the same fully qualified name. That changes both the generated API type and parcel encoding, so existing VINTF clients cannot interoperate with it. Keep the enum and add a new type, or publish a new interface version.
  broadcast/current/docs/broadcast.md:32
• The possessive pronoun is misspelled in this documentation sentence.
  broadcast/current/interface.yaml:7
• This adds a ringbuffer@current dependency, but the current-component manifest does not list either broadcast or ringbuffer, so manifest builds cannot stage this dependency. Add both modules to versions_current.yaml (with the dependency ordering expected by the build) so the broadcast target is buildable in the supported manifest workflow.
  broadcast/metadata.yaml:45
• The scope description contains a misspelling in the name of the data acquisition responsibility.
  broadcast/metadata.yaml:44
• This metadata claims that frontend state changes are delivered to registered listeners, but the new Broadcast API contains no listener interface or listener-registration method. Remove this scope item or restore the event-delivery API so the component metadata matches the contract.
  format_aidl.sh:9
• The formatter only traverses broadcast, so the newly added ringbuffer AIDL files are excluded from the repository's formatting check/workflow. Add the ringbuffer directory to FORMAT_DIRS so both new interfaces are formatted consistently.
  ringbuffer/current/CMakeLists.txt:124
• This standalone target installs to lib/halif, but the repository staging contract and downstream build scripts use out/target/lib/rdk-halif-aidl (see cmake_stage_modules.cmake:52 and avbuffer/current/CMakeLists.txt:157-162). A successful install here therefore leaves the library outside the SDK path consumed by dependants. Use the standard HALIF_INSTALL_LIBDIR default and update the lib/halif examples in this file as well.
  ringbuffer/current/CMakeLists.txt:117
• The target links through ${BINDER_SDK_DIR} but leaves CMake's build RPATH enabled. The Yocto recipe copies the build-tree .so directly, so this embeds an absolute SDK path in the shipped ringbuffer library and can fail package QA; clear the build/install RPATH as done in avclock/current/CMakeLists.txt:133-148.
  ringbuffer/current/CMakeLists.txt:113
• ringbuffer/interface.yaml declares imports: [], and this target links only binder and utils, so requiring HALIF_LIB_DIR makes a standalone RingBuffer configure fail unless an unrelated HAL library directory is supplied. Remove this mandatory check and the dependency link-directory setup for this dependency-free component.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:33
• These lines refer to waitForData, but neither IRingBufferSink nor IRingBufferSource defines that method; both expose acquire(). This makes the core memory-access contract point clients at an API that does not exist.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:41
• This paragraph repeats the nonexistent waitForData method even though the only access method is acquire(), so clients following the documented file-descriptor semantics cannot map the description to the API.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:71
• The non-overflowing behavior is documented here as blocking the producer, but IRingBufferSink.acquire() below is documented to return null when the buffer is full. These are incompatible client contracts; describe the non-overflowing case in terms of the null acquire result.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:85
• The project documentation standard requires @returns rather than @return for Doxygen return descriptions.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:104
• The project documentation standard requires @returns rather than @return for Doxygen return descriptions.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:48
• Unlike the other main HAL interfaces (for example broadcast/.../IBroadcastManager.aidl:36), IRingBuffer has no serviceName constant. This leaves service registration without the contract-defined name required by the repository's AIDL interface convention. Add the constant with the agreed lowercase service name.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:64
• This says an overflowing buffer's acquire returns the requested byte count, but RingBufferAcquireResult.bytes is explicitly allowed to be smaller than the request and remaining exists for the wrapped/contiguous case. Producers must be told to use the returned byte count rather than assume the request was granted.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:71
• The project documentation standard requires @returns, not @return, and the article before RingBufferAcquireResult is incorrect.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:67
• RingBufferAcquireResult has no overflow field. Overflow is represented by RingBufferErrorCode.OVERFLOW delivered through the listener, so this documentation directs consumers to inspect a value that cannot be compiled or read.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:30
• This description has an incorrect article and spelling for the type-safe ID.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:37
• The error-code documentation uses the non-word acquiry instead of the API term acquire.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferInfo.aidl:40
• The field documentation misspells overflow, which is the public setting represented by this field.
  ringbuffer/current/interface.yaml:1
• The new ringbuffer component is not present in versions_current.yaml, so build_modules.sh manifest --file versions_current.yaml will not build or stage it. That manifest mode is the repository's dev cohort and the smoke test expects one library for every current/interface.yaml; it also means the broadcast import of ringbuffer@current has no dependency library in that build. Add both current components to the manifest.
  ringbuffer/metadata.yaml:62
• The reviewer team key is misspelled, so the recorded reviewed status will not match the Broadcast_Team owner key used above and may be ignored by metadata tooling.

• Files reviewed: 101/101 changed files
• Comments generated: 11
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved API compatibility, AIDL validity, build integration, and documentation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (56)

broadcast/current/CMakeLists.txt:136

• This installs the library under lib/halif, while the repository's staging/build scripts and sibling module CMake files use lib/rdk-halif-aidl (for example, deepsleep/current/CMakeLists.txt:144). A direct install therefore places the broadcast library outside the SDK/Yocto package path; use the configurable HALIF_INSTALL_LIBDIR with the repository default.

install(TARGETS ${LIB_NAME}
    LIBRARY DESTINATION lib/halif
            PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
                        GROUP_READ GROUP_EXECUTE
                        WORLD_READ WORLD_EXECUTE)

broadcast/current/CMakeLists.txt:129

• Because this target uses target_link_directories() with absolute Binder and HALIF paths, the default CMake build RPATH embeds local SDK/build directories into the shipped .so. The sibling module CMake files disable this (SKIP_BUILD_RPATH) to satisfy Yocto packaging and keep runtime lookup under the integrator's control; apply the same RPATH configuration here.

set_target_properties(${LIB_NAME} PROPERTIES OUTPUT_NAME "${LIB_NAME}")

broadcast/current/CMakeLists.txt:31

• The standalone build example points HALIF_LIB_DIR at out/target/lib/halif, while the repository's dependency libraries are staged under out/target/lib/rdk-halif-aidl. Following this command makes the broadcast link step unable to find the ringbuffer library. Keep the example aligned with the standard install destination.

#         -DHALIF_LIB_DIR=/path/to/out/target/lib/halif \

broadcast/current/CMakeLists.txt:121

• The configure error still directs users to the obsolete lib/halif directory. When the standard staging path is used, this message gives an invalid recovery command and can leave HALIF_LIB_DIR pointing at the wrong location; update it to lib/rdk-halif-aidl along with the install and example paths.

        "Set it to the directory holding dependency lib*-cpp.so (e.g. out/target/lib/halif).")

broadcast/current/com/rdk/hal/broadcast/IBroadcastManager.aidl:45

• getImplementationVersion() is added before the existing frontend-manager methods, and the old demux-opening API is removed/replaced. That changes the transaction numbers and reply contract of IBroadcastManager, so clients generated against the previous interface will call the wrong operation. Keep the old method order/signatures and append compatible additions, or publish this as a new interface version.

    ImplementationVersion getImplementationVersion();

broadcast/current/com/rdk/hal/broadcast/ca/PowerControl.aidl:27

• Use the UK spelling initialised in this new documentation comment.

    /** Clean value when default initialized. */

broadcast/current/com/rdk/hal/broadcast/demux/FilterType.aidl:35

• Use the UK spelling initialised in this new documentation comment.

    /** Clean value when default initialized */

broadcast/current/com/rdk/hal/broadcast/demux/IDemux.aidl:46

• The existing demux contract is replaced here: the prior source/filter lifecycle is removed in favour of getId(), isConnected(), provider connections, and software inputs. Since this is the same VINTF interface, old generated clients will use the old transaction numbers against unrelated new methods. Preserve IDemux and add a versioned interface for the new lifecycle instead of changing it in place.

    /** Get the ID of this demux. */
    Id getId();

broadcast/current/com/rdk/hal/broadcast/frontend/AtscModulation.aidl:27

• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbCAnnex.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbCBandwidth.aidl:31
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbCModulation.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSModulation.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSRollOff.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSStandard.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:73
• ConstantInt.INVALID_PLS_CODE is an enum member, but this field is an int. AIDL does not use a typed enum member as a primitive parcelable default, so this declaration will fail type checking or generate an invalid assignment; use the numeric -1 default or expose a real const int instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbSTuneParameters.aidl:67
• ConstantInt.INVALID_INPUT_STREAM_ID is an enum member, but this field is an int. AIDL does not use a typed enum member as a primitive parcelable default, so this declaration will fail type checking or generate an invalid assignment; use the numeric -1 default or expose a real const int instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTBandwidth.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTConstellation.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTGuardInterval.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTHierarchy.aidl:18
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTMiso.aidl:18
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTStandard.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTTransmissionMode.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/DvbTTuneParameters.aidl:75
• ConstantInt.INVALID_PLP_ID is an enum member, but this field is an int. AIDL does not use a typed enum member as a primitive parcelable default, so this declaration will fail type checking or generate an invalid assignment; use the numeric -2 default or expose a real const int instead.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendCapabilities.aidl:41
• Use the UK spelling centre in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/FrontendType.aidl:32
• This replaces the existing FrontendType enum values and renumbers the retained DVB values while removing previously supported types such as ATSC3, ISDB, and DTMB. Because the enum backing integers cross the AIDL boundary, old clients can interpret a returned value as the wrong frontend type. Preserve the old values and append new types, or expose the reduced set under a new version/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontend.aidl:50
• This is a breaking rewrite of an existing VINTF interface: getId() and isOpen() are inserted before getFrontendTypes(), while the capability return type and the open/close and listener APIs are changed below. AIDL transaction codes follow declaration order, so existing generated clients will dispatch to different methods or fail to decode replies. Preserve the existing interface and append only compatible methods, or expose this contract as a new version/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/IFrontendController.aidl:60
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/LnbTone.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/LnbVoltage.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalDetectMode.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:31
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoProperty.aidl:41
• Adding FRONTEND_TYPE and ACTUAL_FREQUENCY before the existing values renumbers every pre-existing SignalInfoProperty value; several existing names are also changed (RFLEVEL/UNC). These backing integers are serialized across Binder, so old clients will request/read different properties. Keep the old numeric assignments and names, append new values, or version the API.
  broadcast/current/com/rdk/hal/broadcast/frontend/SignalInfoValue.aidl:51
• This changes the existing SignalInfoValue union's wire contract by reordering/renaming members and changing payload types (for example, actualFrequencyHz to actualFrequency, lock booleans to enums, and RF level to float). Existing clients will decode the old union tags with the wrong type or meaning. Preserve the old members/tags and append new representations, or introduce a versioned union.
  broadcast/current/com/rdk/hal/broadcast/frontend/SpectralInversion.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneParameters.aidl:34
• These union members rename the existing DVB-T/C fields and insert DVB-S in the middle of the union. Union member tags are part of the AIDL wire format, so existing clients serializing dvbTTuneParameters or dvbCTuneParameters will be decoded as a different member/type. Preserve the existing members and tags, append DVB-S, or publish a new versioned union/interface.
  broadcast/current/com/rdk/hal/broadcast/frontend/TuneStatus.aidl:27
• Use the UK spelling initialised in this new documentation comment.
  broadcast/current/docs/broadcast.md:52
• The new frontend API defines FrontendCapabilities.minFrequency and maxFrequency, but this example still points readers to the removed frontend.Capabilities.minFrequencyHz and maxFrequencyHz symbols. Following this documentation leads clients to types and fields that do not exist.
  broadcast/current/docs/broadcast.md:32
• it's is the contraction of “it is”; the possessive form needed here is its.
  broadcast/current/interface.yaml:2
• Adding this interface without registering broadcast in versions_current.yaml means the manifest-mode development build will omit it, while the smoke test still counts it as a current interface. Add the component to the manifest so the new library is built and the expected-count check remains consistent.
  broadcast/metadata.yaml:45
• The metadata uses the misspelling acquission, which makes the public component description unclear.
  format_aidl.sh:8
• This new formatter only traverses broadcast, so running the repository formatting command leaves every AIDL file added under ringbuffer untouched. Add the new module to FORMAT_DIRS so the formatter covers all AIDL trees introduced by this change.
  ringbuffer/current/CMakeLists.txt:117
• Because this target uses target_link_directories() with absolute Binder paths, the default CMake build RPATH embeds the local SDK/build directory into the shipped .so. The sibling module CMake files disable this (SKIP_BUILD_RPATH) to satisfy Yocto packaging and keep runtime lookup under the integrator's control; apply the same RPATH configuration here.
  ringbuffer/current/CMakeLists.txt:124
• This installs the library under lib/halif, while the repository's staging/build scripts and sibling module CMake files use lib/rdk-halif-aidl (for example, deepsleep/current/CMakeLists.txt:144). A direct install therefore places this library outside the SDK/Yocto package path; use the configurable HALIF_INSTALL_LIBDIR with the repository default.
  ringbuffer/current/CMakeLists.txt:113
• ringbuffer/current/interface.yaml declares no HAL imports and this target links only binder/utils, so there is no HALIF dependency to locate here. Making HALIF_LIB_DIR mandatory causes a standalone ringbuffer configure to fail unless callers provide an unrelated directory, even though the component has no dependency library. Remove this requirement and the conditional link-directory block for ringbuffer.
  ringbuffer/current/CMakeLists.txt:31
• The standalone build example points HALIF_LIB_DIR at lib/halif, but this repository stages dependencies under lib/rdk-halif-aidl. Following the documented command leaves the linker unable to find the dependency libraries; update the example to the standard path.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:63
• Use the UK spelling behaviour in this new documentation comment.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBuffer.aidl:48
• IRingBuffer is the module's main service interface, but it has no serviceName constant for service-manager registration. Add the standard constant so clients and implementations have a stable registration name.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSink.aidl:62
• Use the UK spelling behaviour in this new documentation comment.
  ringbuffer/current/com/rdk/hal/ringbuffer/IRingBufferSource.aidl:67
• The documentation says overflow is reported through RingBufferAcquireResult.overflow, but that parcelable only declares id, offset, bytes, and remaining; there is no such field for a consumer to inspect. Either add and define the missing flag or update this contract to describe the actual OVERFLOW error notification.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferAcquireResult.aidl:58
• IRingBufferSource.acquire() documents that callers must inspect RingBufferAcquireResult.overflow to detect discontinuous data, but this parcelable has no such field. Consumers therefore cannot implement the overflow handling promised by the source contract. Add the indicator as a new trailing field, or remove and rewrite the overflow semantics in IRingBufferSource.
  ringbuffer/current/com/rdk/hal/ringbuffer/RingBufferErrorCode.aidl:31
• Use the UK spelling initialised in this new documentation comment.
  ringbuffer/current/interface.yaml:2
• The manifest-based builds only process components listed in versions_current.yaml/versions_released.yaml, but neither manifest includes ringbuffer or broadcast. As a result this new dependency and the broadcast library are skipped by those builds (and the smoke test's component count becomes inconsistent); add both components to the manifests with the appropriate current/release entries.
  ringbuffer/current/mkdocs.yml:1
• This new component-level MkDocs file is not referenced by the root mkdocs.yml, unlike the other component entries, so the ringbuffer documentation will not appear in the published site. Add a root navigation/include entry for ringbuffer/current/mkdocs.yml.

• Files reviewed: 101/101 changed files
• Comments generated: 1
• Review effort level: Lite