[PATCH v6 0/3] cec: Deck Control tests

[PATCH v6 0/3] cec: Deck Control tests

[Deborah Brouwer]

This is part of an Outreachy project to expand the testing of
Deck Control messages as handled by CEC adapters.

Changes since v5:
	cec-compliance:
	- check for Feature Abort before getting deck status.
	cec-follower:
	- send updates to the logical address of the device that
		requested ongoing status updates which may be
		different from the device that requests deck status
		on a one-time basis.

Changes since v4:
	cec-compliance:
	- Change deck_status to a reference and initialize to 0.
	- Revise test results after receiving Feature Abort with
		Incorrect Mode.
	- Simplify for loop for Skip Forward and Reverse.
	- Remove Play as a possible response to Eject.
	- Rename "match" and its argument,  add fail on abort.
	- Rearrange play tests to see clearly all three options
		(MIN/MED/MAX).
	cec-follower:
	- Set deck_skip_start to 0 if a new command or standby occurs
		in the interval between Skip Forward/Reverse and Play.
	- Move skip timer to the end of while loop in testProcessing.
	- Add helper function update_deck_state.

Changes since v3:
	cec-compliance:
		- Stop using REQ_On for monitoring deck status changes; 
			instead add a helper function to get and return deck status.
		- Allow Stop or Eject to return Feature Abort, Incorrect Mode.
		- Replace passive util_receive with an active deck query to see
			if the deck status changes to Play after Skip Forward/Reverse.
		- Add helper function to match play mode and expected deck status.
		- Remove the Deck Status test.
	cec-follower:
		- Track the elasped time since Skip Forward/Reverse and Play after 2s.
		- Remove tray open/close toggle from Eject

Changes since v2:
	cec-compliance:
		- If a deck returns Feature Abort, Incorrect Mode, just provide info
			unless the deck actually has media, then issue a warning.
		- If a deck reports Skip Forward/Reverse status, wait until the status
			changes again before resuming testing to avoid prematurely
			failing the test.
		- Rearrange/change the tests to trigger deck status changes.
	cec-follower:
		- Only report actual status changes, not just every Deck Control
			message that is processed.
		- Send Skip Forward/Reverse and then sleep 2 seconds before
			sending Play.
		- Remove the toggle between Play Forward/Play Still.

Changes since v1:
	- Remove unnecessary functions and node states.
	- Assume that media is present and use the "No Media"
		deck state solely to indicate whether the tray is open.
	- Change and add invalid operands so operands just
		outside of the valid range are tested.
	- Remove restriction to playback/record device.

Deborah Brouwer (3):
  cec: add tests for Deck Control message
  cec: add tests for Deck Play message
  cec-compliance: remove Deck Status test

 utils/cec-compliance/cec-compliance.h |   5 +
 utils/cec-compliance/cec-test.cpp     | 213 ++++++++++++++++++++++----
 utils/cec-follower/cec-follower.cpp   |   2 +
 utils/cec-follower/cec-follower.h     |   2 +
 utils/cec-follower/cec-processing.cpp | 138 ++++++++++++++++-
 5 files changed, 325 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v6 1/3] cec: add tests for Deck Control message

[Deborah Brouwer]

Send all Deck Control commands Skip Forward, Skip Reverse, Stop and Eject
and check the corresponding deck status. Test that the follower returns
Feature Abort for invalid Deck Control operands.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-compliance.h |   5 ++
 utils/cec-compliance/cec-test.cpp     | 111 +++++++++++++++++++++++---
 utils/cec-follower/cec-follower.cpp   |   2 +
 utils/cec-follower/cec-follower.h     |   2 +
 utils/cec-follower/cec-processing.cpp |  70 +++++++++++++++-
 5 files changed, 178 insertions(+), 12 deletions(-)

diff --git a/utils/cec-compliance/cec-compliance.h b/utils/cec-compliance/cec-compliance.h
index fc50e6d9..818181ab 100644
--- a/utils/cec-compliance/cec-compliance.h
+++ b/utils/cec-compliance/cec-compliance.h
@@ -359,6 +359,11 @@ static inline bool refused(const struct cec_msg *msg)
 	return cec_msg_status_is_abort(msg) && abort_reason(msg) == CEC_OP_ABORT_REFUSED;
 }
 
+static inline bool incorrect_mode(const struct cec_msg *msg)
+{
+	return cec_msg_status_is_abort(msg) && abort_reason(msg) == CEC_OP_ABORT_INCORRECT_MODE;
+}
+
 static inline bool timed_out(const struct cec_msg *msg)
 {
 	return msg->rx_status & CEC_RX_STATUS_TIMEOUT;
diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 65a3943a..83be6b73 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -19,6 +19,20 @@ struct remote_test {
 	const vec_remote_subtests &subtests;
 };
 
+static int deck_status_get(struct node *node, unsigned me, unsigned la, __u8 &deck_status)
+{
+	struct cec_msg msg = {};
+	deck_status = 0;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_deck_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(timed_out_or_abort(&msg));
+	cec_ops_deck_status(&msg, &deck_status);
+
+	return OK;
+}
+
 
 /* System Information */
 
@@ -688,24 +702,92 @@ static int deck_ctl_deck_status(struct node *node, unsigned me, unsigned la, boo
 static int deck_ctl_deck_ctl(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	__u8 deck_status;
 
 	cec_msg_init(&msg, me, la);
 	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_STOP);
 	fail_on_test(!transmit_timeout(node, &msg));
-	if (is_playback_or_rec(la)) {
-		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 && !unrecognized_op(&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 && !unrecognized_op(&msg));
 	if (unrecognized_op(&msg))
 		return OK_NOT_SUPPORTED;
 	if (refused(&msg))
 		return OK_REFUSED;
-	if (cec_msg_status_is_abort(&msg))
-		return OK_PRESUMED;
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	if (cec_msg_status_is_abort(&msg)) {
+		if (incorrect_mode(&msg)) {
+			if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
+				info("Stop: no media.\n");
+			else
+				warn("Deck has media but returned Feature Abort with Incorrect Mode.");
+			return OK;
+		}
+		return FAIL;
+	}
+	fail_on_test(deck_status != CEC_OP_DECK_INFO_STOP && deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
 
-	return OK_PRESUMED;
+	cec_msg_init(&msg, me, la);
+	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_SKIP_FWD);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	/*
+	 * If there is no media, Skip Forward should Feature Abort with Incorrect Mode
+	 * even if Stop did not.  If Skip Forward does not Feature Abort, the deck
+	 * is assumed to have media.
+	 */
+	if (incorrect_mode(&msg)) {
+		fail_on_test(deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
+		return OK;
+	}
+	fail_on_test(cec_msg_status_is_abort(&msg));
+	for (unsigned i = 0; deck_status == CEC_OP_DECK_INFO_SKIP_FWD && i < long_timeout; i++) {
+		sleep(1);
+		fail_on_test(deck_status_get(node, me, la, deck_status));
+	}
+	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_SKIP_REV);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(cec_msg_status_is_abort(&msg)); /* Assumes deck has media. */
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	for (unsigned i = 0; deck_status == CEC_OP_DECK_INFO_SKIP_REV && i < long_timeout; i++) {
+		sleep(1);
+		fail_on_test(deck_status_get(node, me, la, deck_status));
+	}
+	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_EJECT);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(cec_msg_status_is_abort(&msg));
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	fail_on_test(deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
+
+	return OK;
+}
+
+static int deck_ctl_deck_ctl_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_deck_control(&msg, 0); /* Invalid Deck Control 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_deck_control(&msg, 5); /* Invalid Deck Control 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_play(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -743,8 +825,17 @@ static const vec_remote_subtests deck_ctl_subtests{
 		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 },
+	{
+		"Deck Control",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_deck_ctl,
+	},
+	{
+		"Deck Control Invalid Operand",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_deck_ctl_invalid,
+	},
 };
 
 /* Tuner Control */
diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 047d7a04..ff47d698 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -314,7 +314,9 @@ void state_init(struct node &node)
 	node.state.volume = 50;
 	node.state.mute = false;
 	node.state.deck_report_changes = false;
+	node.state.deck_report_changes_to = 0;
 	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
+	node.state.deck_skip_start = 0;
 	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 0492faa9..68ef222a 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -50,7 +50,9 @@ struct state {
 	bool service_by_dig_id;
 	bool tuner_report_changes;
 	bool deck_report_changes;
+	__u8 deck_report_changes_to;
 	__u8 deck_state;
+	__u64 deck_skip_start;
 	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 3d2e4a2c..4c7ba1d4 100644
--- a/utils/cec-follower/cec-processing.cpp
+++ b/utils/cec-follower/cec-processing.cpp
@@ -32,6 +32,9 @@
 /* The maximum interval in nanoseconds between audio rate messages as defined in the spec */
 #define MAX_AUD_RATE_MSG_INTERVAL_NS (2 * 1000000000ULL)
 
+/* The maximum interval in nanoseconds to allow a deck to skip forward/reverse */
+#define MAX_DECK_SKIP_NS (2 * 1000000000ULL)
+
 struct cec_enum_values {
 	const char *type_name;
 	__u8 value;
@@ -161,6 +164,7 @@ static bool enter_standby(struct node *node)
 		node->state.old_power_status = node->state.power_status;
 		node->state.power_status = CEC_OP_POWER_STATUS_STANDBY;
 		node->state.power_status_changed_time = time(nullptr);
+		node->state.deck_skip_start = 0;
 		dev_info("Changing state to standby\n");
 		return true;
 	}
@@ -252,6 +256,22 @@ static void aud_rate_msg_interval_check(struct node *node, __u64 ts_new)
 	}
 }
 
+static void update_deck_state(struct node *node, __u8 deck_state_new)
+{
+	if (node->state.deck_state != deck_state_new) {
+		node->state.deck_state = deck_state_new;
+
+		if (node->state.deck_report_changes) {
+
+			struct cec_msg msg = {};
+
+			msg.msg[0] = node->state.deck_report_changes_to;
+			cec_msg_deck_status(&msg, node->state.deck_state);
+			transmit(node, &msg);
+		}
+	}
+}
+
 static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 {
 	__u8 to = cec_msg_destination(&msg);
@@ -517,6 +537,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 		switch (status_req) {
 		case CEC_OP_STATUS_REQ_ON:
 			node->state.deck_report_changes = true;
+			node->state.deck_report_changes_to = (cec_msg_destination(&msg) << 4) | cec_msg_initiator(&msg);
 			fallthrough;
 		case CEC_OP_STATUS_REQ_ONCE:
 			cec_msg_set_reply_to(&msg, &msg);
@@ -525,6 +546,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 			return;
 		case CEC_OP_STATUS_REQ_OFF:
 			node->state.deck_report_changes = false;
+			node->state.deck_report_changes_to = 0;
 			return;
 		default:
 			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
@@ -535,9 +557,48 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 			return;
 		break;
 	case CEC_MSG_DECK_CONTROL:
-		if (node->has_deck_ctl)
+		if (!node->has_deck_ctl)
+			break;
+
+		__u8 deck_state;
+		__u8 deck_control_mode;
+
+		cec_ops_deck_control(&msg, &deck_control_mode);
+
+		switch (deck_control_mode) {
+		case CEC_OP_DECK_CTL_MODE_STOP:
+			deck_state = CEC_OP_DECK_INFO_STOP;
+			node->state.deck_skip_start = 0;
+			break;
+		case CEC_OP_DECK_CTL_MODE_SKIP_FWD:
+			/* Skip Forward will not retract the deck tray. */
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_SKIP_FWD;
+			node->state.deck_skip_start = msg.rx_ts;
+			break;
+		case CEC_OP_DECK_CTL_MODE_SKIP_REV:
+			/* Skip Reverse will not retract the deck tray. */
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_SKIP_REV;
+			node->state.deck_skip_start = msg.rx_ts;
+			break;
+		case CEC_OP_DECK_CTL_MODE_EJECT:
+			deck_state = CEC_OP_DECK_INFO_NO_MEDIA;
+			node->state.deck_skip_start = 0;
+			break;
+		default:
+			cec_msg_reply_feature_abort(&msg, CEC_OP_ABORT_INVALID_OP);
+			transmit(node, &msg);
 			return;
-		break;
+		}
+		update_deck_state(node, deck_state);
+		return;
 	case CEC_MSG_DECK_STATUS:
 		return;
 
@@ -1034,6 +1095,11 @@ void testProcessing(struct node *node, bool wallclock)
 
 		if (node->has_aud_rate)
 			aud_rate_msg_interval_check(node, ts_now);
+
+		if (node->state.deck_skip_start && ts_now - node->state.deck_skip_start > MAX_DECK_SKIP_NS) {
+			node->state.deck_skip_start = 0;
+			update_deck_state(node, CEC_OP_DECK_INFO_PLAY);
+		}
 	}
 	mode = CEC_MODE_INITIATOR;
 	doioctl(node, CEC_S_MODE, &mode);
-- 
2.25.1

[PATCH v6 2/3] cec: add tests for Deck Play message

[Deborah Brouwer]

Send all Deck Play commands and check that the follower implements them.
Test that the follower returns Feature Abort for invalid Play operands.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-test.cpp     | 100 +++++++++++++++++++++++---
 utils/cec-follower/cec-processing.cpp |  70 +++++++++++++++++-
 2 files changed, 156 insertions(+), 14 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 83be6b73..f2ec4e90 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -33,6 +33,20 @@ static int deck_status_get(struct node *node, unsigned me, unsigned la, __u8 &de
 	return OK;
 }
 
+static int test_play_mode(struct node *node, unsigned me, unsigned la, __u8 play_mode, __u8 expected)
+{
+	struct cec_msg msg = {};
+	__u8 deck_status;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_play(&msg, play_mode);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(cec_msg_status_is_abort(&msg)); /* Assumes deck has media. */
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	fail_on_test(deck_status != expected);
+
+	return OK;
+}
 
 /* System Information */
 
@@ -793,24 +807,79 @@ static int deck_ctl_deck_ctl_invalid(struct node *node, unsigned me, unsigned la
 static int deck_ctl_play(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	__u8 deck_status;
 
 	cec_msg_init(&msg, me, la);
-	cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_STILL);
+	cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_FWD);
 	fail_on_test(!transmit_timeout(node, &msg));
-	if (is_playback_or_rec(la)) {
-		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 && !unrecognized_op(&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 && !unrecognized_op(&msg));
 	if (unrecognized_op(&msg))
 		return OK_NOT_SUPPORTED;
 	if (refused(&msg))
 		return OK_REFUSED;
-	if (cec_msg_status_is_abort(&msg))
-		return OK_PRESUMED;
+	fail_on_test(deck_status_get(node, me, la, deck_status));
+	if (cec_msg_status_is_abort(&msg)) {
+		if (incorrect_mode(&msg)) {
+			if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
+				info("Play Still: no media.\n");
+			else
+				warn("Deck has media but returned Feature Abort with Incorrect Mode.");
+			return OK;
+		}
+		return FAIL;
+	}
+	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
 
-	return OK_PRESUMED;
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_STILL, CEC_OP_DECK_INFO_STILL));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_REV, CEC_OP_DECK_INFO_PLAY_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN, CEC_OP_DECK_INFO_FAST_FWD));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN, CEC_OP_DECK_INFO_FAST_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED, CEC_OP_DECK_INFO_FAST_FWD));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED, CEC_OP_DECK_INFO_FAST_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX, CEC_OP_DECK_INFO_FAST_FWD));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX, CEC_OP_DECK_INFO_FAST_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN, CEC_OP_DECK_INFO_SLOW));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN, CEC_OP_DECK_INFO_SLOW_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED, CEC_OP_DECK_INFO_SLOW));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED, CEC_OP_DECK_INFO_SLOW_REV));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX, CEC_OP_DECK_INFO_SLOW));
+	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX, CEC_OP_DECK_INFO_SLOW_REV));
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_STOP);
+	fail_on_test(!transmit_timeout(node, &msg));
+
+	return OK;
+}
+
+static int deck_ctl_play_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_play(&msg, 4); /* 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_play(&msg, 8); /* 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);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_play(&msg, 0x26); /* 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 const vec_remote_subtests deck_ctl_subtests{
@@ -825,7 +894,6 @@ static const vec_remote_subtests deck_ctl_subtests{
 		deck_ctl_give_status_invalid,
 	},
 	{ "Deck Status", CEC_LOG_ADDR_MASK_ALL, deck_ctl_deck_status },
-	{ "Play", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_play },
 	{
 		"Deck Control",
 		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
@@ -836,6 +904,16 @@ static const vec_remote_subtests deck_ctl_subtests{
 		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
 		deck_ctl_deck_ctl_invalid,
 	},
+	{
+		"Play",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_play,
+	},
+	{
+		"Play Invalid Operand",
+		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
+		deck_ctl_play_invalid,
+	},
 };
 
 /* Tuner Control */
diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
index 4c7ba1d4..e58df85f 100644
--- a/utils/cec-follower/cec-processing.cpp
+++ b/utils/cec-follower/cec-processing.cpp
@@ -553,14 +553,78 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 			return;
 		}
 	case CEC_MSG_PLAY:
-		if (node->has_deck_ctl)
+		if (!node->has_deck_ctl)
+			break;
+
+		__u8 deck_state;
+		__u8 play_mode;
+
+		cec_ops_play(&msg, &play_mode);
+
+		switch (play_mode) {
+		case CEC_OP_PLAY_MODE_PLAY_FWD:
+			/* Play Forward will close tray if open. */
+			deck_state = CEC_OP_DECK_INFO_PLAY;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_REV:
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_PLAY_REV;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_STILL:
+			/* Play Still will close tray if open. */
+			deck_state = CEC_OP_DECK_INFO_STILL;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN:
+		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED:
+		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX:
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_FAST_FWD;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN:
+		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED:
+		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX:
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_FAST_REV;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN:
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED:
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX:
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_SLOW;
+			break;
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN:
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED:
+		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX:
+			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
+				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
+				return;
+			}
+			deck_state = CEC_OP_DECK_INFO_SLOW_REV;
+			break;
+		default:
+			cec_msg_reply_feature_abort(&msg, CEC_OP_ABORT_INVALID_OP);
+			transmit(node, &msg);
 			return;
-		break;
+		}
+		node->state.deck_skip_start = 0;
+		update_deck_state(node, deck_state);
+		return;
 	case CEC_MSG_DECK_CONTROL:
 		if (!node->has_deck_ctl)
 			break;
 
-		__u8 deck_state;
 		__u8 deck_control_mode;
 
 		cec_ops_deck_control(&msg, &deck_control_mode);
-- 
2.25.1

[PATCH v6 3/3] cec-compliance: remove Deck Status test

[Deborah Brouwer]

Remove the Deck Status message test because invalid Deck Status messages
are already captured by cec-compliance in other tests.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index f2ec4e90..8f2188ff 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -696,23 +696,6 @@ static int deck_ctl_give_status_invalid(struct node *node, unsigned me, unsigned
 	return OK;
 }
 
-static int deck_ctl_deck_status(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
-
-	cec_msg_init(&msg, me, la);
-	cec_msg_deck_status(&msg, CEC_OP_DECK_INFO_STOP);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return OK_NOT_SUPPORTED;
-	if (refused(&msg))
-		return OK_REFUSED;
-	if (cec_msg_status_is_abort(&msg))
-		return OK_PRESUMED;
-
-	return 0;
-}
-
 static int deck_ctl_deck_ctl(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
@@ -893,7 +876,6 @@ static const vec_remote_subtests deck_ctl_subtests{
 		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,
-- 
2.25.1

Re: [PATCH v6 1/3] cec: add tests for Deck Control message

[Hans Verkuil]

Hi Deb,

Some minor comments:

On 11/06/2021 01:58, Deborah Brouwer wrote:
> Send all Deck Control commands Skip Forward, Skip Reverse, Stop and Eject
> and check the corresponding deck status. Test that the follower returns
> Feature Abort for invalid Deck Control operands.
> 
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
> ---
>  utils/cec-compliance/cec-compliance.h |   5 ++
>  utils/cec-compliance/cec-test.cpp     | 111 +++++++++++++++++++++++---
>  utils/cec-follower/cec-follower.cpp   |   2 +
>  utils/cec-follower/cec-follower.h     |   2 +
>  utils/cec-follower/cec-processing.cpp |  70 +++++++++++++++-
>  5 files changed, 178 insertions(+), 12 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-compliance.h b/utils/cec-compliance/cec-compliance.h
> index fc50e6d9..818181ab 100644
> --- a/utils/cec-compliance/cec-compliance.h
> +++ b/utils/cec-compliance/cec-compliance.h
> @@ -359,6 +359,11 @@ static inline bool refused(const struct cec_msg *msg)
>  	return cec_msg_status_is_abort(msg) && abort_reason(msg) == CEC_OP_ABORT_REFUSED;
>  }
>  
> +static inline bool incorrect_mode(const struct cec_msg *msg)
> +{
> +	return cec_msg_status_is_abort(msg) && abort_reason(msg) == CEC_OP_ABORT_INCORRECT_MODE;
> +}
> +
>  static inline bool timed_out(const struct cec_msg *msg)
>  {
>  	return msg->rx_status & CEC_RX_STATUS_TIMEOUT;
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 65a3943a..83be6b73 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -19,6 +19,20 @@ struct remote_test {
>  	const vec_remote_subtests &subtests;
>  };
>  
> +static int deck_status_get(struct node *node, unsigned me, unsigned la, __u8 &deck_status)
> +{
> +	struct cec_msg msg = {};
> +	deck_status = 0;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_give_deck_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(timed_out_or_abort(&msg));
> +	cec_ops_deck_status(&msg, &deck_status);
> +
> +	return OK;
> +}
> +
>  
>  /* System Information */
>  
> @@ -688,24 +702,92 @@ static int deck_ctl_deck_status(struct node *node, unsigned me, unsigned la, boo
>  static int deck_ctl_deck_ctl(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	__u8 deck_status;
>  
>  	cec_msg_init(&msg, me, la);
>  	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_STOP);
>  	fail_on_test(!transmit_timeout(node, &msg));
> -	if (is_playback_or_rec(la)) {
> -		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 && !unrecognized_op(&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 && !unrecognized_op(&msg));
>  	if (unrecognized_op(&msg))
>  		return OK_NOT_SUPPORTED;
>  	if (refused(&msg))
>  		return OK_REFUSED;
> -	if (cec_msg_status_is_abort(&msg))
> -		return OK_PRESUMED;
> +	fail_on_test(deck_status_get(node, me, la, deck_status));
> +	if (cec_msg_status_is_abort(&msg)) {
> +		if (incorrect_mode(&msg)) {
> +			if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
> +				info("Stop: no media.\n");
> +			else
> +				warn("Deck has media but returned Feature Abort with Incorrect Mode.");
> +			return OK;
> +		}
> +		return FAIL;

Let's reorder this a bit:

		if (!incorrect_mode(&msg))
			return FAIL;
		if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
			info("Stop: no media.\n");
		else
			warn("Deck has media but returned Feature Abort with Incorrect Mode.");
		return OK;

It looks nicer that way.	

> +	}
> +	fail_on_test(deck_status != CEC_OP_DECK_INFO_STOP && deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
>  
> -	return OK_PRESUMED;
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_SKIP_FWD);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(deck_status_get(node, me, la, deck_status));
> +	/*
> +	 * If there is no media, Skip Forward should Feature Abort with Incorrect Mode
> +	 * even if Stop did not.  If Skip Forward does not Feature Abort, the deck
> +	 * is assumed to have media.
> +	 */
> +	if (incorrect_mode(&msg)) {
> +		fail_on_test(deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
> +		return OK;
> +	}
> +	fail_on_test(cec_msg_status_is_abort(&msg));

Add a short comment here explaining what the for loop does.

> +	for (unsigned i = 0; deck_status == CEC_OP_DECK_INFO_SKIP_FWD && i < long_timeout; i++) {
> +		sleep(1);
> +		fail_on_test(deck_status_get(node, me, la, deck_status));
> +	}
> +	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_SKIP_REV);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(cec_msg_status_is_abort(&msg)); /* Assumes deck has media. */
> +	fail_on_test(deck_status_get(node, me, la, deck_status));

Ditto.

> +	for (unsigned i = 0; deck_status == CEC_OP_DECK_INFO_SKIP_REV && i < long_timeout; i++) {
> +		sleep(1);
> +		fail_on_test(deck_status_get(node, me, la, deck_status));
> +	}
> +	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_EJECT);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(cec_msg_status_is_abort(&msg));
> +	fail_on_test(deck_status_get(node, me, la, deck_status));
> +	fail_on_test(deck_status != CEC_OP_DECK_INFO_NO_MEDIA);
> +
> +	return OK;
> +}
> +
> +static int deck_ctl_deck_ctl_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> +	struct cec_msg msg = {};
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_deck_control(&msg, 0); /* Invalid Deck Control 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_deck_control(&msg, 5); /* Invalid Deck Control 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_play(struct node *node, unsigned me, unsigned la, bool interactive)
> @@ -743,8 +825,17 @@ static const vec_remote_subtests deck_ctl_subtests{
>  		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 },
> +	{
> +		"Deck Control",
> +		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> +		deck_ctl_deck_ctl,
> +	},
> +	{
> +		"Deck Control Invalid Operand",
> +		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> +		deck_ctl_deck_ctl_invalid,
> +	},
>  };
>  
>  /* Tuner Control */
> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
> index 047d7a04..ff47d698 100644
> --- a/utils/cec-follower/cec-follower.cpp
> +++ b/utils/cec-follower/cec-follower.cpp
> @@ -314,7 +314,9 @@ void state_init(struct node &node)
>  	node.state.volume = 50;
>  	node.state.mute = false;
>  	node.state.deck_report_changes = false;
> +	node.state.deck_report_changes_to = 0;
>  	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
> +	node.state.deck_skip_start = 0;
>  	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 0492faa9..68ef222a 100644
> --- a/utils/cec-follower/cec-follower.h
> +++ b/utils/cec-follower/cec-follower.h
> @@ -50,7 +50,9 @@ struct state {
>  	bool service_by_dig_id;
>  	bool tuner_report_changes;
>  	bool deck_report_changes;
> +	__u8 deck_report_changes_to;
>  	__u8 deck_state;
> +	__u64 deck_skip_start;
>  	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 3d2e4a2c..4c7ba1d4 100644
> --- a/utils/cec-follower/cec-processing.cpp
> +++ b/utils/cec-follower/cec-processing.cpp
> @@ -32,6 +32,9 @@
>  /* The maximum interval in nanoseconds between audio rate messages as defined in the spec */
>  #define MAX_AUD_RATE_MSG_INTERVAL_NS (2 * 1000000000ULL)
>  
> +/* The maximum interval in nanoseconds to allow a deck to skip forward/reverse */
> +#define MAX_DECK_SKIP_NS (2 * 1000000000ULL)
> +
>  struct cec_enum_values {
>  	const char *type_name;
>  	__u8 value;
> @@ -161,6 +164,7 @@ static bool enter_standby(struct node *node)
>  		node->state.old_power_status = node->state.power_status;
>  		node->state.power_status = CEC_OP_POWER_STATUS_STANDBY;
>  		node->state.power_status_changed_time = time(nullptr);
> +		node->state.deck_skip_start = 0;
>  		dev_info("Changing state to standby\n");
>  		return true;
>  	}
> @@ -252,6 +256,22 @@ static void aud_rate_msg_interval_check(struct node *node, __u64 ts_new)
>  	}
>  }
>  
> +static void update_deck_state(struct node *node, __u8 deck_state_new)
> +{
> +	if (node->state.deck_state != deck_state_new) {
> +		node->state.deck_state = deck_state_new;
> +
> +		if (node->state.deck_report_changes) {
> +

Drop this newline.

> +			struct cec_msg msg = {};
> +
> +			msg.msg[0] = node->state.deck_report_changes_to;

Use cec_msg_init for this. Also pass 'me' (initiator) as argument to this function.

> +			cec_msg_deck_status(&msg, node->state.deck_state);
> +			transmit(node, &msg);
> +		}
> +	}
> +}
> +
>  static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  {
>  	__u8 to = cec_msg_destination(&msg);
> @@ -517,6 +537,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  		switch (status_req) {
>  		case CEC_OP_STATUS_REQ_ON:
>  			node->state.deck_report_changes = true;
> +			node->state.deck_report_changes_to = (cec_msg_destination(&msg) << 4) | cec_msg_initiator(&msg);

Just set this to cec_msg_initiator(&msg). Setting it to both is confusing, and
in fact conflicts with the name 'deck_report_changes_to': that indicates that just the
destination LA is stored.

>  			fallthrough;
>  		case CEC_OP_STATUS_REQ_ONCE:
>  			cec_msg_set_reply_to(&msg, &msg);
> @@ -525,6 +546,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  			return;
>  		case CEC_OP_STATUS_REQ_OFF:
>  			node->state.deck_report_changes = false;
> +			node->state.deck_report_changes_to = 0;

You can drop this: deck_report_changes_to is only used if deck_report_changes is
true, so no need to clear deck_report_changes_to.

>  			return;
>  		default:
>  			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
> @@ -535,9 +557,48 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  			return;
>  		break;
>  	case CEC_MSG_DECK_CONTROL:
> -		if (node->has_deck_ctl)
> +		if (!node->has_deck_ctl)
> +			break;
> +
> +		__u8 deck_state;
> +		__u8 deck_control_mode;
> +
> +		cec_ops_deck_control(&msg, &deck_control_mode);
> +
> +		switch (deck_control_mode) {
> +		case CEC_OP_DECK_CTL_MODE_STOP:
> +			deck_state = CEC_OP_DECK_INFO_STOP;
> +			node->state.deck_skip_start = 0;
> +			break;
> +		case CEC_OP_DECK_CTL_MODE_SKIP_FWD:
> +			/* Skip Forward will not retract the deck tray. */
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_SKIP_FWD;
> +			node->state.deck_skip_start = msg.rx_ts;
> +			break;
> +		case CEC_OP_DECK_CTL_MODE_SKIP_REV:
> +			/* Skip Reverse will not retract the deck tray. */
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_SKIP_REV;
> +			node->state.deck_skip_start = msg.rx_ts;
> +			break;
> +		case CEC_OP_DECK_CTL_MODE_EJECT:
> +			deck_state = CEC_OP_DECK_INFO_NO_MEDIA;
> +			node->state.deck_skip_start = 0;
> +			break;
> +		default:
> +			cec_msg_reply_feature_abort(&msg, CEC_OP_ABORT_INVALID_OP);
> +			transmit(node, &msg);
>  			return;
> -		break;
> +		}
> +		update_deck_state(node, deck_state);
> +		return;
>  	case CEC_MSG_DECK_STATUS:
>  		return;
>  
> @@ -1034,6 +1095,11 @@ void testProcessing(struct node *node, bool wallclock)
>  
>  		if (node->has_aud_rate)
>  			aud_rate_msg_interval_check(node, ts_now);
> +
> +		if (node->state.deck_skip_start && ts_now - node->state.deck_skip_start > MAX_DECK_SKIP_NS) {
> +			node->state.deck_skip_start = 0;
> +			update_deck_state(node, CEC_OP_DECK_INFO_PLAY);
> +		}
>  	}
>  	mode = CEC_MODE_INITIATOR;
>  	doioctl(node, CEC_S_MODE, &mode);
> 

Regards,

	Hans

Re: [PATCH v6 2/3] cec: add tests for Deck Play message

[Hans Verkuil]

Hi Deb,

Some more (small) comments:

On 11/06/2021 01:58, Deborah Brouwer wrote:
> Send all Deck Play commands and check that the follower implements them.
> Test that the follower returns Feature Abort for invalid Play operands.
> 
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp     | 100 +++++++++++++++++++++++---
>  utils/cec-follower/cec-processing.cpp |  70 +++++++++++++++++-
>  2 files changed, 156 insertions(+), 14 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 83be6b73..f2ec4e90 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -33,6 +33,20 @@ static int deck_status_get(struct node *node, unsigned me, unsigned la, __u8 &de
>  	return OK;
>  }
>  
> +static int test_play_mode(struct node *node, unsigned me, unsigned la, __u8 play_mode, __u8 expected)
> +{
> +	struct cec_msg msg = {};
> +	__u8 deck_status;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_play(&msg, play_mode);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(cec_msg_status_is_abort(&msg)); /* Assumes deck has media. */
> +	fail_on_test(deck_status_get(node, me, la, deck_status));
> +	fail_on_test(deck_status != expected);
> +
> +	return OK;
> +}
>  
>  /* System Information */
>  
> @@ -793,24 +807,79 @@ static int deck_ctl_deck_ctl_invalid(struct node *node, unsigned me, unsigned la
>  static int deck_ctl_play(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	__u8 deck_status;
>  
>  	cec_msg_init(&msg, me, la);
> -	cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_STILL);
> +	cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_FWD);
>  	fail_on_test(!transmit_timeout(node, &msg));
> -	if (is_playback_or_rec(la)) {
> -		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 && !unrecognized_op(&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 && !unrecognized_op(&msg));
>  	if (unrecognized_op(&msg))
>  		return OK_NOT_SUPPORTED;
>  	if (refused(&msg))
>  		return OK_REFUSED;
> -	if (cec_msg_status_is_abort(&msg))
> -		return OK_PRESUMED;
> +	fail_on_test(deck_status_get(node, me, la, deck_status));
> +	if (cec_msg_status_is_abort(&msg)) {
> +		if (incorrect_mode(&msg)) {
> +			if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
> +				info("Play Still: no media.\n");
> +			else
> +				warn("Deck has media but returned Feature Abort with Incorrect Mode.");
> +			return OK;
> +		}
> +		return FAIL;

Do the same reordering as I suggested in the previous patch.

> +	}
> +	fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
>  
> -	return OK_PRESUMED;
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_STILL, CEC_OP_DECK_INFO_STILL));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_REV, CEC_OP_DECK_INFO_PLAY_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN, CEC_OP_DECK_INFO_FAST_FWD));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN, CEC_OP_DECK_INFO_FAST_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED, CEC_OP_DECK_INFO_FAST_FWD));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED, CEC_OP_DECK_INFO_FAST_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX, CEC_OP_DECK_INFO_FAST_FWD));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX, CEC_OP_DECK_INFO_FAST_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN, CEC_OP_DECK_INFO_SLOW));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN, CEC_OP_DECK_INFO_SLOW_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED, CEC_OP_DECK_INFO_SLOW));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED, CEC_OP_DECK_INFO_SLOW_REV));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX, CEC_OP_DECK_INFO_SLOW));
> +	fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX, CEC_OP_DECK_INFO_SLOW_REV));
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_STOP);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +
> +	return OK;
> +}
> +
> +static int deck_ctl_play_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> +	struct cec_msg msg = {};
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_play(&msg, 4); /* 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_play(&msg, 8); /* Invalid Operand */

I think it would be more useful to test with 0 instead of 8.

I did consider testing for both, but I think that is a bit overkill.

> +	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);
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_play(&msg, 0x26); /* 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 const vec_remote_subtests deck_ctl_subtests{
> @@ -825,7 +894,6 @@ static const vec_remote_subtests deck_ctl_subtests{
>  		deck_ctl_give_status_invalid,
>  	},
>  	{ "Deck Status", CEC_LOG_ADDR_MASK_ALL, deck_ctl_deck_status },
> -	{ "Play", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_play },
>  	{
>  		"Deck Control",
>  		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> @@ -836,6 +904,16 @@ static const vec_remote_subtests deck_ctl_subtests{
>  		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
>  		deck_ctl_deck_ctl_invalid,
>  	},
> +	{
> +		"Play",
> +		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> +		deck_ctl_play,
> +	},
> +	{
> +		"Play Invalid Operand",
> +		CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> +		deck_ctl_play_invalid,
> +	},
>  };
>  
>  /* Tuner Control */
> diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
> index 4c7ba1d4..e58df85f 100644
> --- a/utils/cec-follower/cec-processing.cpp
> +++ b/utils/cec-follower/cec-processing.cpp
> @@ -553,14 +553,78 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  			return;
>  		}
>  	case CEC_MSG_PLAY:
> -		if (node->has_deck_ctl)
> +		if (!node->has_deck_ctl)
> +			break;
> +
> +		__u8 deck_state;
> +		__u8 play_mode;
> +
> +		cec_ops_play(&msg, &play_mode);
> +
> +		switch (play_mode) {
> +		case CEC_OP_PLAY_MODE_PLAY_FWD:
> +			/* Play Forward will close tray if open. */
> +			deck_state = CEC_OP_DECK_INFO_PLAY;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_REV:
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_PLAY_REV;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_STILL:
> +			/* Play Still will close tray if open. */
> +			deck_state = CEC_OP_DECK_INFO_STILL;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN:
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED:
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX:
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_FAST_FWD;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN:
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED:
> +		case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX:
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_FAST_REV;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN:
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED:
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX:
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_SLOW;
> +			break;
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN:
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED:
> +		case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX:
> +			if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> +				reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> +				return;
> +			}
> +			deck_state = CEC_OP_DECK_INFO_SLOW_REV;
> +			break;
> +		default:
> +			cec_msg_reply_feature_abort(&msg, CEC_OP_ABORT_INVALID_OP);
> +			transmit(node, &msg);
>  			return;
> -		break;
> +		}

The NO_MEDIA checks are repeated several times in the switch above. I think that it
is better to do this here:

		if (play_mode != CEC_OP_PLAY_MODE_PLAY_FWD &&
		    play_mode != CEC_OP_PLAY_MODE_PLAY_STILL &&
		    node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
			reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
			return;
		}

> +		node->state.deck_skip_start = 0;
> +		update_deck_state(node, deck_state);
> +		return;
>  	case CEC_MSG_DECK_CONTROL:
>  		if (!node->has_deck_ctl)
>  			break;
>  
> -		__u8 deck_state;
>  		__u8 deck_control_mode;
>  
>  		cec_ops_deck_control(&msg, &deck_control_mode);
> 

Regards,

	Hans