feat: HDMI support on Ripple #264

chakradhar29 · 2023-10-04T20:08:33Z

What

Adds HDMI support on Ripple

Why

HDMI support on Ripple

How

Integrates with AVInput thunder api running on RDK devices

Test

Sends requests through firebolt hdmi module

Checklist

I have self-reviewed this PR
I have added tests that prove the feature works or the fix is effective

CLAassistant · 2023-10-04T20:08:39Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ chakradhar29
❌ kevinshahfws
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

… handlers

…la-RPPL-1151_supportHdmiOnRipple

satlead · 2023-10-06T18:46:46Z

core/main/src/firebolt/handlers/hdmi_rpc.rs

+
+#[rpc(server)]
+pub trait Hdmi {
+    #[method(name = "hdmi.setActiveInput")]


HDMIInput.select

Updated it.

satlead · 2023-10-06T18:47:08Z

core/sdk/src/api/firebolt/panel/fb_hdmi.rs

+#[derive(Debug, Serialize, Deserialize, Clone)]
+#[serde(rename_all = "camelCase")]
+pub struct StartHdmiInputRequest {
+    pub id: String,


Enumeration for Start and Stop

pub operation: HDMIOperation

Add a new enum

#[derive(Debug, Serialize, Deserialize, Clone)] #[serde(rename_all = "camelCase")] pub enum HdmiOperation { Start, Stop }

Added enum.

satlead · 2023-10-06T18:50:40Z

device/thunder_ripple_sdk/src/processors/panel/thunder_hdmi.rs

+        let response = state
+            .get_thunder_client()
+            .call(DeviceCallRequest {
+                method: ThunderPlugin::AVInput.method("startInput"),


Use HdmiOperation to change the method to stopInput

…la-RPPL-1151_supportHdmiOnRipple

…llumuntala-RPPL-1151_supportHdmiOnRipple

core/main/src/firebolt/handlers/hdmi_rpc.rs

core/sdk/src/api/firebolt/panel/fb_hdmi.rs

adamdama · 2023-10-09T14:28:25Z

core/main/src/firebolt/handlers/hdmi_rpc.rs

+            .await
+            .is_err()
+        {
+            error!("Error while registration");


Should we still return an OK response even if we hit this block?

This is similar to how on_hdcp_changed listener is implemented

Ripple/core/main/src/firebolt/handlers/device_rpc.rs

Line 417 in 0565eee

.is_err()

Can you elaborate on how I should change it?

Might need some input from @satlead on this one. However, from my point of view here the code reads:

Send DeviceEventRequest to the extension that will handle it

If we get an error then log it

Tell the caller that everything worked ok

What I would expect is this:

Send DeviceEventRequest to the extension that will handle it

If we get an error then log it and tell the caller it didn't work

If we didn't get an error then tell the caller that everything worked ok

So if this is the same in HDCP then I would think that might need looking at as well (in a different PR of course).

I think that a more appropriate implementation would be:

Suggested change

error!("Error while registration");

error!("Error while registration");

return Err(Error::Custom("Failed to subscribe to HdmiConnectionChanged event".to_owned()));

Note: you will want to add a use statement for Error e.g. use jsonrpsee::core::Error;.

@satlead please could you confirm I am on the right track with this one?

Got it. thank you :)

Looks like there is a util function rpc_error that can be used to glean it up a bit:

return Err(Error::Custom("Failed to subscribe to HdmiConnectionChanged event".to_owned()));

Becomes:

return Err(rpc_error("Failed to subscribe to HdmiConnectionChanged event"));

Sorry, I didn't know about this fn until just now.

I believe it's rpc_err. Used it, thanks.

adamdama · 2023-10-09T14:29:40Z

core/main/src/firebolt/handlers/hdmi_rpc.rs

+        if let HdmiOperation::Start = request.operation {
+            if let Ok(response) = self
+                .state
+                .get_client()
+                .send_extn_request(HdmiRequest::StartHdmiInput(request))
+                .await
+            {
+                if let ExtnPayload::Response(ExtnResponse::Value(value)) = response.payload {
+                    if let Ok(res) = serde_json::from_value::<HdmiSelectOperationResponse>(value) {
+                        return Ok(res);
+                    }
+                }
+            }
+
+            Err(rpc_err("FB error response TBD"))
+        } else {
+            if let Ok(response) = self
+                .state
+                .get_client()
+                .send_extn_request(HdmiRequest::StopHdmiInput(request))
+                .await
+            {
+                if let ExtnPayload::Response(ExtnResponse::Value(value)) = response.payload {
+                    if let Ok(res) = serde_json::from_value::<HdmiSelectOperationResponse>(value) {
+                        return Ok(res);
+                    }
+                }
+            }
+
+            Err(rpc_err("FB error response TBD"))
+        }


The if/else block branches look the same to me. Is that right?

Yes needs some cleanup by passing the operation to the Device handler

Fixed it in latest commit 01ea440#diff-130906b50b27c33d5f205f3da7521a976f98012695e1c65d5882619ddf96973fR73

core/main/src/firebolt/handlers/hdmi_rpc.rs

adamdama · 2023-10-09T14:32:35Z

core/sdk/src/api/device/panel/device_hdmi.rs

+pub enum AVInputAdjective {
+    Hdmi,
+    Ota,
+    Composite,
+}


Why is this defined in an HDMI module? When implementing Composite for example I would not expect to find the adjective for it defined here in HDMI.

yeah AVInput probably needs a super .rs file for OTA and composite

Moved AVInputAdjective to a super .rs file.

adamdama · 2023-10-09T14:36:19Z

device/thunder_ripple_sdk/src/processors/panel/thunder_hdmi.rs

+        }
+    }
+
+    async fn get_available_inputs(state: ThunderState, req: ExtnMessage) -> bool {


All of the handlers look basically the same. Is there a way we can reduce the repeated code?

Certainly Start and Stop can be just one value in the operation which is what was suggested. Get available inputs body is slightly different so there would be something additional.

Think we can move the Thunder call alone to a separate function to reduce the boilerplate

Refactored it in latest code.

…llumuntala-RPPL-1151_supportHdmiOnRipple

…la-RPPL-1151_supportHdmiOnRipple

…//github.com/rdkcentral/Ripple into cchillumuntala-RPPL-1151_supportHdmiOnRipple

…la-RPPL-1151_supportHdmiOnRipple

….com:rdkcentral/Ripple into cchillumuntala-RPPL-1151_supportHdmiOnRipple

core/sdk/src/api/firebolt/panel/fb_hdmi.rs

2. Renamed fb_hdmi to fb_hdmi_input

…la-RPPL-1151_supportHdmiOnRipple

…ration response

…la-RPPL-1151_supportHdmiOnRipple

feat(RPPL-1151): HDMI support on Ripple

535f2a7

chakradhar29 changed the title ~~feat(RPPL-1151): HDMI support on Ripple~~ feat: HDMI support on Ripple Oct 5, 2023

chakradhar29 added 2 commits October 6, 2023 12:52

feat: Added onConnectionChanged and onAutoLowLatencyModeSignalChanged…

0d98646

… handlers

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

e7ac902

…la-RPPL-1151_supportHdmiOnRipple

satlead added the do not merge Use for PRs which are experimental and testing purposes. label Oct 6, 2023

satlead reviewed Oct 6, 2023

View reviewed changes

chakradhar29 and others added 3 commits October 6, 2023 16:07

feat: Added hdmi start and stop operations

0ca9174

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

e7ce159

…la-RPPL-1151_supportHdmiOnRipple

Merge branch 'main' of https://github.com/rdkcentral/Ripple into cchi…

a5fde43

…llumuntala-RPPL-1151_supportHdmiOnRipple

adamdama reviewed Oct 9, 2023

View reviewed changes

core/main/src/firebolt/handlers/hdmi_rpc.rs Outdated Show resolved Hide resolved

adamdama reviewed Oct 9, 2023

View reviewed changes

core/sdk/src/api/firebolt/panel/fb_hdmi.rs Outdated Show resolved Hide resolved

adamdama reviewed Oct 9, 2023

View reviewed changes

core/main/src/firebolt/handlers/hdmi_rpc.rs Outdated Show resolved Hide resolved

adamdama reviewed Oct 9, 2023

View reviewed changes

kevinshahfws and others added 11 commits October 9, 2023 11:16

Merge branch 'main' of https://github.com/rdkcentral/Ripple into cchi…

4b28530

…llumuntala-RPPL-1151_supportHdmiOnRipple

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

41295c4

…la-RPPL-1151_supportHdmiOnRipple

Addressing code review comments

c0d743d

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

0565eee

…la-RPPL-1151_supportHdmiOnRipple

Addressed code review comments #2

01ea440

Addressed code review comments #3

05dc748

Contract test for HdmiInput.getInputDevice

90efc53

Merge branch 'cchillumuntala-RPPL-1151_supportHdmiOnRipple' of https:…

a6b7cdd

…//github.com/rdkcentral/Ripple into cchillumuntala-RPPL-1151_supportHdmiOnRipple

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

fbef7fe

…la-RPPL-1151_supportHdmiOnRipple

Addressed code review comments #4

25a2e80

Merge branch 'cchillumuntala-RPPL-1151_supportHdmiOnRipple' of github…

8b69eaf

….com:rdkcentral/Ripple into cchillumuntala-RPPL-1151_supportHdmiOnRipple

Fixed formatting error causing build failure

dd6c956

chakradhar29 marked this pull request as ready for review October 10, 2023 19:15

adamdama reviewed Oct 11, 2023

View reviewed changes

core/sdk/src/api/firebolt/panel/fb_hdmi.rs Outdated Show resolved Hide resolved

chakradhar29 added 12 commits October 11, 2023 10:29

1. Returning error when listener registration failed.

c81db36

2. Renamed fb_hdmi to fb_hdmi_input

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

713b32c

…la-RPPL-1151_supportHdmiOnRipple

Added license header

20dc822

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

1c05a9c

…la-RPPL-1151_supportHdmiOnRipple

Used a different error method

0b69ca6

fix: GetAvailableInputsResponse was being used to parse HdmiSelectOpe…

4500f4e

…ration response

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

44495a8

…la-RPPL-1151_supportHdmiOnRipple

Added port method, avinput contract

6596395

Merge branch 'main' of github.com:rdkcentral/Ripple into cchillumunta…

a00734d

…la-RPPL-1151_supportHdmiOnRipple

Making hdmi firebolt calls work

96dcc86

Making HDMIInput.port work

4eae11c

Commenting on hdmi.port method

c54f484

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HDMI support on Ripple #264

feat: HDMI support on Ripple #264

chakradhar29 commented Oct 4, 2023

CLAassistant commented Oct 4, 2023 •

edited

Loading

satlead Oct 6, 2023

chakradhar29 Oct 10, 2023

satlead Oct 6, 2023 •

edited

Loading

chakradhar29 Oct 10, 2023

satlead Oct 6, 2023

chakradhar29 Oct 10, 2023

adamdama Oct 9, 2023

chakradhar29 Oct 10, 2023

adamdama Oct 11, 2023

chakradhar29 Oct 11, 2023

adamdama Oct 13, 2023

chakradhar29 Oct 13, 2023

adamdama Oct 9, 2023

satlead Oct 9, 2023

chakradhar29 Oct 10, 2023

adamdama Oct 9, 2023

satlead Oct 9, 2023

chakradhar29 Oct 10, 2023

adamdama Oct 9, 2023

satlead Oct 9, 2023

chakradhar29 Oct 10, 2023

	error!("Error while registration");
	error!("Error while registration");
	return Err(Error::Custom("Failed to subscribe to HdmiConnectionChanged event".to_owned()));

feat: HDMI support on Ripple #264

Are you sure you want to change the base?

feat: HDMI support on Ripple #264

Conversation

chakradhar29 commented Oct 4, 2023

What

Why

How

Test

Checklist

CLAassistant commented Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satlead Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 4, 2023 •

edited

Loading

satlead Oct 6, 2023 •

edited

Loading