[PATCH v4 0/2] cec: add tests for Give Deck Status message

[PATCH v4 0/2] cec: add tests for Give Deck Status message

[Deborah Brouwer]

Changes since v3:
	Patch 1/2:
		- Remove the CEC Version checks and else block.
		- Add comment explaining why it works for CEC 1.4.
	Patch 2/2:
		- Keep/add empty line between variable declarations and code.
		- Remove restriction to playback or recording devices.
		- Extract the deck status where it was previously missed.
		- Extend comment to explain CEC_OP_STATUS_REQ_OFF test.
		- In the invalid test, remove redundant testing.

Changes since v2:
	Patch 1/2 cec-follower: emulate features for CEC versions < CEC 2.0
	* I already sent this as a single patch, and I have not made
		changes since then, I am just including it because now I
		need it for Patch 2/2 to apply.

	Patch 2/2 cec: add tests for Give Deck Status message
	* Combine functions with same name "deck_ctl_give_status"
		to avoid confusion about the naming but also to avoid
		returning FAIL rather than using "fail_on_test" directly.
	* Expand the "Invalid Operand" test so that it will also test a
		follower running version < CEC 2.0.
	* Change the invalid operand from 0xaa to 0 and add a test for
		invalid operand "4", both just outside of the valid range.
	* Expand the "Invalid Operand" test to catch the other reasons
		for which a follower might Feature Abort (e.g. Unrecognized
		Op).

Changes since v1:
	* Remove unnecessary functions.
	* Revise function for turning Give Deck Status reporting On/Off.
	* Combine the Give Deck Status Reporting test into the first
	   Give Deck Status test.

Deborah Brouwer (2):
  cec-follower: emulate features for CEC versions < CEC 2.0
  cec: add tests for Give Deck Status message

 utils/cec-compliance/cec-test.cpp     | 72 ++++++++++++++++++++++-----
 utils/cec-follower/cec-follower.cpp   | 44 +++++++++-------
 utils/cec-follower/cec-follower.h     |  1 +
 utils/cec-follower/cec-processing.cpp | 41 +++++++--------
 utils/libcecutil/cec-info.cpp         |  2 -
 5 files changed, 104 insertions(+), 56 deletions(-)

--
2.17.1

[PATCH v4 1/2] cec-follower: emulate features for CEC versions < CEC 2.0

[Deborah Brouwer]

For CEC adapters using versions < CEC 2.0, read the CEC Features
as configured in the CEC adapter and emulate the features that are
present.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-follower/cec-follower.cpp | 42 ++++++++++++++++-------------
 utils/libcecutil/cec-info.cpp       |  2 --
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index b7a41ac2..1f598fdf 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -508,26 +508,30 @@ int main(int argc, char **argv)
 
 	cec_driver_info(caps, laddrs, node.phys_addr, conn_info);
 
-	if (laddrs.cec_version >= CEC_OP_CEC_VERSION_2_0) {
-		bool is_dev_feat = false;
-
-		for (__u8 byte : laddrs.features[0]) {
-			if (is_dev_feat) {
-				node.source_has_arc_rx = (byte & CEC_OP_FEAT_DEV_SOURCE_HAS_ARC_RX) != 0;
-				node.sink_has_arc_tx = (byte & CEC_OP_FEAT_DEV_SINK_HAS_ARC_TX) != 0;
-				node.has_aud_rate = (byte & CEC_OP_FEAT_DEV_HAS_SET_AUDIO_RATE) != 0;
-				node.has_deck_ctl = (byte & CEC_OP_FEAT_DEV_HAS_DECK_CONTROL) != 0;
-				node.has_rec_tv = (byte & CEC_OP_FEAT_DEV_HAS_RECORD_TV_SCREEN) != 0;
-				node.has_osd_string = (byte & CEC_OP_FEAT_DEV_HAS_SET_OSD_STRING) != 0;
-				break;
-			}
-			if (byte & CEC_OP_FEAT_EXT)
-				continue;
-			if (!is_dev_feat)
-				is_dev_feat = true;
-			else
-				break;
+	/*
+	 * For CEC 1.4, features of a logical address may still be
+	 * filled in according to the CEC 2.0 guidelines even though
+	 * the CEC framework won’t use the features in the CEC 2.0
+	 * CEC_MSG_REPORT_FEATURES.
+	 */
+	bool is_dev_feat = false;
+
+	for (__u8 byte : laddrs.features[0]) {
+		if (is_dev_feat) {
+			node.source_has_arc_rx = (byte & CEC_OP_FEAT_DEV_SOURCE_HAS_ARC_RX) != 0;
+			node.sink_has_arc_tx = (byte & CEC_OP_FEAT_DEV_SINK_HAS_ARC_TX) != 0;
+			node.has_aud_rate = (byte & CEC_OP_FEAT_DEV_HAS_SET_AUDIO_RATE) != 0;
+			node.has_deck_ctl = (byte & CEC_OP_FEAT_DEV_HAS_DECK_CONTROL) != 0;
+			node.has_rec_tv = (byte & CEC_OP_FEAT_DEV_HAS_RECORD_TV_SCREEN) != 0;
+			node.has_osd_string = (byte & CEC_OP_FEAT_DEV_HAS_SET_OSD_STRING) != 0;
+			break;
 		}
+		if (byte & CEC_OP_FEAT_EXT)
+			continue;
+		if (!is_dev_feat)
+			is_dev_feat = true;
+		else
+			break;
 	}
 	printf("\n");
 
diff --git a/utils/libcecutil/cec-info.cpp b/utils/libcecutil/cec-info.cpp
index 8b3c55e8..3c768261 100644
--- a/utils/libcecutil/cec-info.cpp
+++ b/utils/libcecutil/cec-info.cpp
@@ -448,8 +448,6 @@ void cec_driver_info(const struct cec_caps &caps,
 		       cec_prim_type2s(laddrs.primary_device_type[i]));
 		printf("\t    Logical Address Type   : %s\n",
 		       cec_la_type2s(laddrs.log_addr_type[i]));
-		if (laddrs.cec_version < CEC_OP_CEC_VERSION_2_0)
-			continue;
 		printf("\t    All Device Types       : %s\n",
 		       cec_all_dev_types2s(laddrs.all_device_types[i]).c_str());
 
-- 
2.17.1

[PATCH v4 2/2] cec: add tests for Give Deck Status message

[Deborah Brouwer]

Test that a valid Deck Status message is received in response to Give Deck
Status and that Give Deck Status automatic reporting can be turned on and
off. Test that the follower returns Feature Abort for an invalid Give Deck
Status operand.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-test.cpp     | 72 ++++++++++++++++++++++-----
 utils/cec-follower/cec-follower.cpp   |  2 +
 utils/cec-follower/cec-follower.h     |  1 +
 utils/cec-follower/cec-processing.cpp | 41 +++++++--------
 4 files changed, 81 insertions(+), 35 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index fb57d10b..4578dd13 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -601,10 +601,6 @@ static const vec_remote_subtests dev_menu_ctl_subtests{
 
 /* Deck Control */
 
-/*
-  TODO: These are very rudimentary tests which should be expanded.
- */
-
 static int deck_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
@@ -613,12 +609,11 @@ static int deck_ctl_give_status(struct node *node, unsigned me, unsigned la, boo
 	cec_msg_give_deck_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
 	fail_on_test(!transmit_timeout(node, &msg));
 	fail_on_test(timed_out(&msg));
-	if (is_playback_or_rec(la)) {
-		fail_on_test_v2(node->remote[la].cec_version,
-				node->remote[la].has_deck_ctl && cec_msg_status_is_abort(&msg));
-		fail_on_test_v2(node->remote[la].cec_version,
-				!node->remote[la].has_deck_ctl && !unrecognized_op(&msg));
-	}
+
+	fail_on_test_v2(node->remote[la].cec_version,
+	                node->remote[la].has_deck_ctl && cec_msg_status_is_abort(&msg));
+	fail_on_test_v2(node->remote[la].cec_version,
+	                !node->remote[la].has_deck_ctl && !unrecognized_op(&msg));
 	if (unrecognized_op(&msg))
 		return OK_NOT_SUPPORTED;
 	if (refused(&msg))
@@ -626,7 +621,51 @@ static int deck_ctl_give_status(struct node *node, unsigned me, unsigned la, boo
 	if (cec_msg_status_is_abort(&msg))
 		return OK_PRESUMED;
 
-	return 0;
+	__u8 deck_info;
+
+	cec_ops_deck_status(&msg, &deck_info);
+	fail_on_test(deck_info < CEC_OP_DECK_INFO_PLAY || deck_info > CEC_OP_DECK_INFO_OTHER);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_deck_status(&msg, true, CEC_OP_STATUS_REQ_ON);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(timed_out(&msg));
+	cec_ops_deck_status(&msg, &deck_info);
+	fail_on_test(deck_info < CEC_OP_DECK_INFO_PLAY || deck_info > CEC_OP_DECK_INFO_OTHER);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_deck_status(&msg, true, CEC_OP_STATUS_REQ_OFF);
+	/*
+	 * Reply would not normally be expected for CEC_OP_STATUS_REQ_OFF.
+	 * If a reply is received, then the follower failed to turn off
+	 * status reporting as required.
+	 */
+	msg.reply = CEC_MSG_DECK_STATUS;
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(!timed_out(&msg));
+
+	return OK;
+}
+
+static int deck_ctl_give_status_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_deck_status(&msg, true, 0); /* Invalid Operand */
+	fail_on_test(!transmit_timeout(node, &msg));
+	if (unrecognized_op(&msg))
+		return OK_NOT_SUPPORTED;
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_deck_status(&msg, true, 4); /* Invalid Operand */
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
+
+	return OK;
 }
 
 static int deck_ctl_deck_status(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -693,7 +732,16 @@ static int deck_ctl_play(struct node *node, unsigned me, unsigned la, bool inter
 }
 
 static const vec_remote_subtests deck_ctl_subtests{
-	{ "Give Deck Status", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_give_status },
+	{
+		"Give Deck Status",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_give_status,
+	},
+	{
+		"Give Deck Status Invalid Operand",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_give_status_invalid,
+	},
 	{ "Deck Status", CEC_LOG_ADDR_MASK_ALL, deck_ctl_deck_status },
 	{ "Deck Control", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_deck_ctl },
 	{ "Play", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_play },
diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 1f598fdf..047d7a04 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -313,6 +313,8 @@ void state_init(struct node &node)
 	node.state.sac_active = false;
 	node.state.volume = 50;
 	node.state.mute = false;
+	node.state.deck_report_changes = false;
+	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
 	tuner_dev_info_init(&node.state);
 	node.state.last_aud_rate_rx_ts = 0;
 }
diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
index 391b9ab4..0492faa9 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -50,6 +50,7 @@ struct state {
 	bool service_by_dig_id;
 	bool tuner_report_changes;
 	bool deck_report_changes;
+	__u8 deck_state;
 	unsigned toggle_power_status;
 	__u64 last_aud_rate_rx_ts;
 };
diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
index 5b9db19f..3d2e4a2c 100644
--- a/utils/cec-follower/cec-processing.cpp
+++ b/utils/cec-follower/cec-processing.cpp
@@ -505,36 +505,31 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 		break;
 
 
-		/*
-		  Deck Control
-
-		  This is only a basic implementation.
-
-		  TODO: Device state should reflect whether we are playing,
-		  fast forwarding, etc.
-		*/
+		/* Deck Control */
 
 	case CEC_MSG_GIVE_DECK_STATUS:
-		if (node->has_deck_ctl) {
-			__u8 status_req;
+		if (!node->has_deck_ctl)
+			break;
 
-			cec_ops_give_deck_status(&msg, &status_req);
-			if (status_req < CEC_OP_STATUS_REQ_ON ||
-			    status_req > CEC_OP_STATUS_REQ_ONCE) {
-				reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
-				return;
-			}
-			if (status_req != CEC_OP_STATUS_REQ_ONCE)
-				node->state.deck_report_changes =
-					status_req == CEC_OP_STATUS_REQ_ON;
-			if (status_req == CEC_OP_STATUS_REQ_OFF)
-				return;
+		__u8 status_req;
+		cec_ops_give_deck_status(&msg, &status_req);
+
+		switch (status_req) {
+		case CEC_OP_STATUS_REQ_ON:
+			node->state.deck_report_changes = true;
+			fallthrough;
+		case CEC_OP_STATUS_REQ_ONCE:
 			cec_msg_set_reply_to(&msg, &msg);
-			cec_msg_deck_status(&msg, CEC_OP_DECK_INFO_STOP);
+			cec_msg_deck_status(&msg, node->state.deck_state);
 			transmit(node, &msg);
 			return;
+		case CEC_OP_STATUS_REQ_OFF:
+			node->state.deck_report_changes = false;
+			return;
+		default:
+			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
+			return;
 		}
-		break;
 	case CEC_MSG_PLAY:
 		if (node->has_deck_ctl)
 			return;
-- 
2.17.1

Re: [PATCH v4 0/2] cec: add tests for Give Deck Status message

[Hans Verkuil]

Hi Deb,

Thank you for your work, I've applied the patches.

One note for future patch series: the cover letter should also explain
a bit about what the patch series is about, not just the change
history. Something to keep in mind for the next patch series.

Regards,

	Hans

On 04/06/2021 04:40, Deborah Brouwer wrote:
> Changes since v3:
> 	Patch 1/2:
> 		- Remove the CEC Version checks and else block.
> 		- Add comment explaining why it works for CEC 1.4.
> 	Patch 2/2:
> 		- Keep/add empty line between variable declarations and code.
> 		- Remove restriction to playback or recording devices.
> 		- Extract the deck status where it was previously missed.
> 		- Extend comment to explain CEC_OP_STATUS_REQ_OFF test.
> 		- In the invalid test, remove redundant testing.
> 
> Changes since v2:
> 	Patch 1/2 cec-follower: emulate features for CEC versions < CEC 2.0
> 	* I already sent this as a single patch, and I have not made
> 		changes since then, I am just including it because now I
> 		need it for Patch 2/2 to apply.
> 
> 	Patch 2/2 cec: add tests for Give Deck Status message
> 	* Combine functions with same name "deck_ctl_give_status"
> 		to avoid confusion about the naming but also to avoid
> 		returning FAIL rather than using "fail_on_test" directly.
> 	* Expand the "Invalid Operand" test so that it will also test a
> 		follower running version < CEC 2.0.
> 	* Change the invalid operand from 0xaa to 0 and add a test for
> 		invalid operand "4", both just outside of the valid range.
> 	* Expand the "Invalid Operand" test to catch the other reasons
> 		for which a follower might Feature Abort (e.g. Unrecognized
> 		Op).
> 
> Changes since v1:
> 	* Remove unnecessary functions.
> 	* Revise function for turning Give Deck Status reporting On/Off.
> 	* Combine the Give Deck Status Reporting test into the first
> 	   Give Deck Status test.
> 
> Deborah Brouwer (2):
>   cec-follower: emulate features for CEC versions < CEC 2.0
>   cec: add tests for Give Deck Status message
> 
>  utils/cec-compliance/cec-test.cpp     | 72 ++++++++++++++++++++++-----
>  utils/cec-follower/cec-follower.cpp   | 44 +++++++++-------
>  utils/cec-follower/cec-follower.h     |  1 +
>  utils/cec-follower/cec-processing.cpp | 41 +++++++--------
>  utils/libcecutil/cec-info.cpp         |  2 -
>  5 files changed, 104 insertions(+), 56 deletions(-)
> 
> --
> 2.17.1
>