gh#164 Update description of APIs from HDMI and Composite - #188
gh#164 Update description of APIs from HDMI and Composite#188Dhivyailangovan wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the public header documentation for HDMI-In and Composite-In APIs to describe asynchronous status updates and the use of status-change callbacks, aligning consumer expectations when reading status immediately after state-changing operations.
Changes:
- Add notes about waiting for status-change callbacks after operations that affect input status.
- Clarify
isPresentedupdate behavior on port selection and add callback-related notes in the select-port docs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| include/dsHdmiIn.h | Adds/adjusts Doxygen notes about asynchronous status updates and callback delivery for HDMI-In status. |
| include/dsCompositeIn.h | Adds/adjusts Doxygen notes about asynchronous status updates and callback delivery for Composite-In status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dhivyailangovan
changed the title
|
Can you check if copilot comments are relevant? if not please resolve it. |
70c76e8 to
187b426
Compare
Addressed Copilot Comments. |
| * | ||
| * @warning This API is Not thread safe. | ||
| * After any operation that may change ::dsHdmiInStatus_t, dsHdmiInGetStatus() | ||
| * may return stale values until ::dsHdmiInStatusChangeCB_t is received. |
There was a problem hiding this comment.
It’s not really a limitation. On Amlogic, there is a short delay (about 2–3 seconds) before mirroring starts. During this time, dsHdmiInGetStatus() may return old values. Once the callback is received, the status will be correct(For any SOC).
| * @retval dsERR_GENERAL - Underlying undefined platform error | ||
| * | ||
| * @warning This API is Not thread safe. | ||
| * After any operation that may change ::dsHdmiInStatus_t, dsHdmiInGetStatus() |
There was a problem hiding this comment.
@Dhivyailangovan , why is HDMI status relevant here for Composite module?
There was a problem hiding this comment.
corrected the lines
| @@ -213,7 +216,9 @@ dsError_t dsHdmiInGetStatus (dsHdmiInStatus_t *pStatus); | |||
| * @pre dsHdmiInInit() must be called before calling this API. | |||
| * | |||
| * @note When a port is selected, activePort should be set to true in Please refer ::dsHdmiInStatus_t for that port | |||
There was a problem hiding this comment.
when a port is selected, activePort should be set to true in? The sentence looks to be incomplete.
There was a problem hiding this comment.
Thank you for pointing this out. I haven’t modified this line or its logic in this PR.
The sentence means that when a port is selected, activePort should be set to true in dsHdmiInStatus_t (please refer to ::dsHdmiInStatus_t for more details).
If you’d like changes or clarification for this part, may we track it separately so it doesn’t block this PR?
There was a problem hiding this comment.
if its a small clarification, let's push it here please
| * @note When a port is selected, activePort should be set to true in Please refer ::dsHdmiInStatus_t for that port | ||
| * Also, if thT port has an active connection, it should update isPresented to true as well. | ||
| * Also, if the port has an active connection, it should update isPresented to true as well. | ||
| * Changes to ::dsHdmiInStatus_t are communicated asynchronously via the |
There was a problem hiding this comment.
This line means that any updates to ::dsHdmiInStatus_t will be reported asynchronously to the application through the ::dsHdmiInStatusChangeCB_t callback function.
This change addresses https://ccp.sys.comcast.net/browse/RDKEVD-456. Currently, dsHdmiInGetStatus is called immediately after dsHdmiInSelectPort in the "L2_VerifyHdmiInputPortStatus" flow, which causes failures because the status may not be updated yet. By adding this explanation, we highlight that the VTS script should wait for the dsHdmiInStatusChangeCB_t callback before checking the status after selecting a port
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * may return stale values until ::dsHdmiInStatusChangeCB_t is received. | ||
| * After the callback is received, dsHdmiInGetStatus() will return the updated status. |
There was a problem hiding this comment.
The wording "until ::dsHdmiInStatusChangeCB_t is received" is ambiguous/misleading because dsHdmiInStatusChangeCB_t is a callback type, not something that is "received". Consider rephrasing to say the registered status-change callback is invoked/called (and optionally that it's delivered asynchronously).
| * may return stale values until ::dsHdmiInStatusChangeCB_t is received. | |
| * After the callback is received, dsHdmiInGetStatus() will return the updated status. | |
| * may return stale values until the ::dsHdmiInStatusChangeCB_t callback has been invoked. | |
| * After the callback has been invoked, dsHdmiInGetStatus() will return the updated status. |
| * @note When a port is selected, activePort should be set to true in Please refer ::dsHdmiInStatus_t for that port | ||
| * Also, if thT port has an active connection, it should update isPresented to true as well. | ||
| * Also, if the port has an active connection, it should update isPresented to true as well. | ||
| * Changes to ::dsHdmiInStatus_t are communicated asynchronously via the |
There was a problem hiding this comment.
This note sentence is grammatically incomplete: "activePort should be set to true in Please refer ::dsHdmiInStatus_t for that port". It reads like two sentences merged together and makes it unclear where activePort is set. Please rewrite for clarity (e.g., explicitly state activePort is set in the corresponding dsHdmiInStatus_t entry for the selected port).
There was a problem hiding this comment.
These updates to say that a status info will be stale until there is a update callback are largely unnecessary.
What is the purpose of the change, why do it?
There is always going to be a race condition where a call to get a status can be different to the callback. This is normal. The callback is asynchronous. The call to get status may just miss the update of the status change and so will be wrong at the time of reading.
The purpose of the callback is to avoid polling of a getStatus() function and so the getStatus() function is allowed to be "stale". At the time of calling the status may not of updated. So what? A callback will eventually arrive with the update and a new read of the status will reflect the callback status unless it coincides with a status change.
Much more important is that get status functions reflects the last callback reported status.
The user should never have to subscribe to a callback for the getstatus functions to get updated. I hope that is the case.
There was a problem hiding this comment.
Having read more deeply I can now see the problem.
The callback is not the issue.
The issue is that the original dsHdmiInGetStatus() function description is ambiguous.
It should state exactly what the status should be immediately after the call to dsHdmiInSelectPort().
There should probably be a status of "unknown" that can be returned when the status has not updated on select port. Or state that it must wait for the status to be internally updated if ok to block.
Or we could state that after a call to dsHdmiInSelectPort() the status will be "not connected" until detected as connected. Many way it could of been defined.
This is the problem.
The implementation does not know what to do.
If it can make a call to HW and get it immediately then it is OK.
But if it is dependent (internally) on an asynchronous CB to update its internal state then there is no defined way of handling it.
In light of the fact that we do not want to change everything now as a result of a fundamental change to the interface definition to remove the ambiguity then I see that changing the documentation to say until the callback has been received the status may be "stale". But rather than "stale" the correct word is "wrong".
Else we have to fix all implementations, test and release.
There was a problem hiding this comment.
Ok, overall, let's not modify the interface for now. Let's handle this from the VTS side, and we will upgrade to tests to wait for the CB before we check the status.
| * @note When a port is selected that port should be set as activePort in ::dsCompositeInStatus_t. | ||
| * Also, if there is a signal (ie isPortConnected[that port ID] is true), once active, isPresented should be set to true as well. | ||
| * | ||
| * Changes to ::dsCompositeInStatus_t are communicated asynchronously via the | ||
| * ::dsCompositeInStatusChangeCB_t callback. | ||
| * |
There was a problem hiding this comment.
PR description/issue mention that activePort and isPresented are updated "asynchronously in a separate thread" and communicated via the status-change callback. The updated docs mention asynchronous callback delivery, but they don't mention the separate-thread aspect or explicitly call out activePort/isPresented. If that detail is required for API consumers, please reflect it here to match the PR description/issue requirements.
outdooruseonly
left a comment
outdooruseonly
left a comment
There was a problem hiding this comment.
Having read more deeply I can now see the problem.
The callback is not the issue.
The issue is that the original dsHdmiInGetStatus() function description is ambiguous.
It should state exactly what the status should be immediately after the call to dsHdmiInSelectPort().
There should probably be a status of "unknown" that can be returned when the status has not updated on select port. Or state that it must wait for the status to be internally updated if ok to block.
Or we could state that after a call to dsHdmiInSelectPort() the status will be "not connected" until detected as connected. Many way it could of been defined.
This is the problem.
The implementation does not know what to do.
If it can make a call to HW and get it immediately then it is OK.
But if it is dependent (internally) on an asynchronous CB to update its internal state then there is no defined way of handling it.
In light of the fact that we do not want to change everything now as a result of a fundamental change to the interface definition to remove the ambiguity then I see that changing the documentation to say until the callback has been received the status may be "stale". But rather than "stale" the correct word is "wrong".
Else we have to fix all implementations, test and release.
| * @note When a port is selected, activePort should be set to true in Please refer ::dsHdmiInStatus_t for that port | ||
| * Also, if thT port has an active connection, it should update isPresented to true as well. | ||
| * Also, if the port has an active connection, it should update isPresented to true as well. | ||
| * Changes to ::dsHdmiInStatus_t are communicated asynchronously via the |
There was a problem hiding this comment.
Having read more deeply I can now see the problem.
The callback is not the issue.
The issue is that the original dsHdmiInGetStatus() function description is ambiguous.
It should state exactly what the status should be immediately after the call to dsHdmiInSelectPort().
There should probably be a status of "unknown" that can be returned when the status has not updated on select port. Or state that it must wait for the status to be internally updated if ok to block.
Or we could state that after a call to dsHdmiInSelectPort() the status will be "not connected" until detected as connected. Many way it could of been defined.
This is the problem.
The implementation does not know what to do.
If it can make a call to HW and get it immediately then it is OK.
But if it is dependent (internally) on an asynchronous CB to update its internal state then there is no defined way of handling it.
In light of the fact that we do not want to change everything now as a result of a fundamental change to the interface definition to remove the ambiguity then I see that changing the documentation to say until the callback has been received the status may be "stale". But rather than "stale" the correct word is "wrong".
Else we have to fix all implementations, test and release.
|
Triage note (#188, ~95 days old, last activity ~65 days ago, BLOCKED): This PR for #164 (Update description of APIs from HDMI and Composite) is doc-only and moderate staleness. Status: BLOCKED. Suggested next step: author check-in. Doc-only PRs tend to slip; a ping from the reviewer side could shake it loose. |
Need to update the description of APIs from HDMI( dsHdmiInGetStatus and dsHdmiInSelectPort) and Composite(dsCompositeInGetStatus and dsCompositeInSelectPort) to explain below points
activePort and isPresented will be updated asynchronously in a separate thread, and will be communicated through the (hdmiInStatusChangeCB/dsCompositeInStatusChangeCB) callback.
Fixes #164