diff mbox series

[v5,3/3] cec: add One Touch Record Standby tests

Message ID 7940ca575845c46d56d25e65b46db55682c719f7.1624578960.git.deborahbrouwer3563@gmail.com
State New
Headers show
Series [v5,1/3] cec-follower: use log_addr_type to get local device type | expand

Commit Message

Deborah Brouwer June 25, 2021, 12:13 a.m. UTC 
Check that the recording device ignores a Standby message while it is
recording. When the recording is finished, check that the recording device
enters standby unless the recording device is the active source.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-test-power.cpp | 63 +++++++++++++++++++++++++
 utils/cec-follower/cec-follower.cpp     |  1 +
 utils/cec-follower/cec-follower.h       |  2 +
 utils/cec-follower/cec-processing.cpp   |  8 +++-
 utils/cec-follower/cec-tuner.cpp        |  9 ++++
 5 files changed, 81 insertions(+), 2 deletions(-)

Comments

Hans Verkuil June 28, 2021, 9:20 a.m. UTC | #1 
Hi Deb,

Sorry for the late review, it was very busy week and it took some time before I had
the time to dig into this. But here is my review:

On 25/06/2021 02:13, Deborah Brouwer wrote:
> Check that the recording device ignores a Standby message while it is

> recording. When the recording is finished, check that the recording device

> enters standby unless the recording device is the active source.

> 

> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>

> ---

>  utils/cec-compliance/cec-test-power.cpp | 63 +++++++++++++++++++++++++

>  utils/cec-follower/cec-follower.cpp     |  1 +

>  utils/cec-follower/cec-follower.h       |  2 +

>  utils/cec-follower/cec-processing.cpp   |  8 +++-

>  utils/cec-follower/cec-tuner.cpp        |  9 ++++

>  5 files changed, 81 insertions(+), 2 deletions(-)

> 

> diff --git a/utils/cec-compliance/cec-test-power.cpp b/utils/cec-compliance/cec-test-power.cpp

> index b675bfc4..c6bf7093 100644

> --- a/utils/cec-compliance/cec-test-power.cpp

> +++ b/utils/cec-compliance/cec-test-power.cpp

> @@ -677,6 +677,67 @@ static int standby_resume_wakeup_deck_play(struct node *node, unsigned me, unsig

>  	return standby_resume_wakeup_deck(node, me, la, interactive, CEC_OP_PLAY_MODE_PLAY_FWD);

>  }

>  

> +static int standby_record(struct node *node, unsigned me, unsigned la, bool interactive, __u8 opcode)

> +{

> +	struct cec_msg msg;

> +	__u8 rec_status;

> +

> +	cec_msg_init(&msg, me, la);

> +	cec_msg_record_on_own(&msg);

> +	msg.reply = CEC_MSG_RECORD_STATUS;

> +	fail_on_test(!transmit_timeout(node, &msg, 10000));

> +	if (timed_out_or_abort(&msg))

> +		return OK_NOT_SUPPORTED;

> +	cec_ops_record_status(&msg, &rec_status);

> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_CUR_SRC &&

> +	             rec_status != CEC_OP_RECORD_STATUS_ALREADY_RECORDING);

> +

> +	cec_msg_init(&msg, me, la);

> +	cec_msg_standby(&msg);

> +	fail_on_test(!transmit_timeout(node, &msg));

> +	/* Standby should not interrupt the recording. */

> +	fail_on_test(!cec_msg_status_is_abort(&msg));

> +

> +	/* When the recording stops, the recorder should standby unless the recorder is the active source */

> +	cec_msg_init(&msg, me, la);

> +	if (opcode == CEC_MSG_ACTIVE_SOURCE)


You don't actually use opcode, you just use it to decide whether to make the
remove device the active source or not. Just change the argument to a bool
and give it a more descriptive name.

> +		cec_msg_active_source(&msg, node->remote[la].phys_addr);

> +	else

> +		cec_msg_active_source(&msg, node->remote[la].phys_addr + 1);


Using phys_addr + 1 is a bit dangerous: what if there actually is such a device?
Instead, use the physical address of 'me'. That's guaranteed to be both valid
and different from the remote physical address.

> +	fail_on_test(!transmit_timeout(node, &msg));


You should transmit this before you transmit the standby. Right now you might
make a device in standby the active source, which is odd.

> +

> +	cec_msg_init(&msg, me, la);

> +	cec_msg_record_off(&msg, false);

> +	fail_on_test(!transmit_timeout(node, &msg));

> +

> +	unsigned unresponsive_time = 0;

> +

> +	if (opcode == CEC_MSG_ACTIVE_SOURCE) {

> +		fail_on_test(!poll_stable_power_status(node, me, la, CEC_OP_POWER_STATUS_ON, unresponsive_time));

> +	} else {

> +		fail_on_test(!poll_stable_power_status(node, me, la, CEC_OP_POWER_STATUS_STANDBY, unresponsive_time));

> +		fail_on_test(interactive && !question("Is the device in standby?"));

> +		node->remote[la].in_standby = true;

> +

> +		int ret = standby_resume_wakeup(node, me, la, interactive);

> +		if (ret)

> +			return ret;

> +		node->remote[la].in_standby = false;

> +	}

> +

> +	return OK;

> +}

> +

> +static int standby_record_active_source(struct node *node, unsigned me, unsigned la, bool interactive)

> +{

> +	return standby_record(node, me, la, interactive, CEC_MSG_ACTIVE_SOURCE);

> +}

> +

> +static int standby_record_inactive_source(struct node *node, unsigned me, unsigned la, bool interactive)

> +{

> +	return standby_record(node, me, la, interactive, CEC_MSG_INACTIVE_SOURCE);

> +}

> +

>  const vec_remote_subtests standby_resume_subtests{

>  	{ "Standby", CEC_LOG_ADDR_MASK_ALL, standby_resume_standby },

>  	{ "Repeated Standby message does not wake up", CEC_LOG_ADDR_MASK_ALL, standby_resume_standby_toggle },

> @@ -697,4 +758,6 @@ const vec_remote_subtests standby_resume_subtests{

>  	{ "Power State Transitions", CEC_LOG_ADDR_MASK_TV, power_state_transitions, false, true },

>  	{ "Deck Eject Standby Resume", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, standby_resume_wakeup_deck_eject },

>  	{ "Deck Play Standby Resume", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, standby_resume_wakeup_deck_play },

> +	{ "Record Standby Active Source", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, standby_record_active_source },

> +	{ "Record Standby Inactive Source", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, standby_record_inactive_source },

>  };

> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp

> index 482192e7..d6c9cdfb 100644

> --- a/utils/cec-follower/cec-follower.cpp

> +++ b/utils/cec-follower/cec-follower.cpp

> @@ -318,6 +318,7 @@ void state_init(struct node &node)

>  	node.state.deck_state = CEC_OP_DECK_INFO_STOP;

>  	node.state.deck_skip_start = 0;

>  	node.state.one_touch_record_on = false;

> +	node.state.one_touch_record_standby = false;

>  	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 3bcb2774..a5d1c3a6 100644

> --- a/utils/cec-follower/cec-follower.h

> +++ b/utils/cec-follower/cec-follower.h

> @@ -54,6 +54,7 @@ struct state {

>  	__u8 deck_state;

>  	__u64 deck_skip_start;

>  	bool one_touch_record_on;

> +	bool one_touch_record_standby;


How about: bool one_touch_record_received_standby;

Perhaps the one_touch_ prefix can be dropped in these two variables.
It will probably depend on the timer feature as well: both deal with
recording, and so these bools might be valid for both features, in
which case the prefix makes no sense.

>  	time_t toggle_power_status;

>  	__u64 last_aud_rate_rx_ts;

>  };

> @@ -230,5 +231,6 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns

>  void reply_feature_abort(struct node *node, struct cec_msg *msg,

>  			 __u8 reason = CEC_OP_ABORT_UNRECOGNIZED_OP);

>  void testProcessing(struct node *node, bool wallclock);

> +bool enter_standby(struct node *node);

>  

>  #endif

> diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp

> index f985a841..43e03878 100644

> --- a/utils/cec-follower/cec-processing.cpp

> +++ b/utils/cec-follower/cec-processing.cpp

> @@ -157,7 +157,7 @@ static bool exit_standby(struct node *node)

>  	return false;

>  }

>  

> -static bool enter_standby(struct node *node)

> +bool enter_standby(struct node *node)

>  {

>  	if (node->state.power_status == CEC_OP_POWER_STATUS_ON ||

>  	    node->state.power_status == CEC_OP_POWER_STATUS_TO_ON) {

> @@ -320,10 +320,14 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me, __u8

>  		/* Standby */

>  

>  	case CEC_MSG_STANDBY:

> +		/* Standby should not interrupt a recording in progress. */


This comment should be extended with: "but set one_touch_record_standby
to true to remember to go to standby once the recording is finished."

> +		if (node->state.one_touch_record_on) {

> +			node->state.one_touch_record_standby = true;

> +			break;

> +		}


Shouldn't exit_standby() set node->state.one_touch_record_standby to false?

>  		enter_standby(node);

>  		return;

>  

> -

>  		/* One Touch Play and Routing Control */

>  

>  	case CEC_MSG_ACTIVE_SOURCE: {

> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp

> index 0f40b7d7..4d159456 100644

> --- a/utils/cec-follower/cec-tuner.cpp

> +++ b/utils/cec-follower/cec-tuner.cpp

> @@ -716,6 +716,15 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns

>  		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_TERMINATED_OK);

>  		transmit(node, &msg);

>  		node->state.one_touch_record_on = false;

> +		/*

> +		 * If standby was received during recording, enter standby when the recording is finished

> +		 * unless the recording device is the currently the active source.

> +		 */

> +		if (node->state.one_touch_record_standby) {

> +			if (node->phys_addr != node->state.active_source_pa)

> +				enter_standby(node);

> +			node->state.one_touch_record_standby = false;

> +		}

>  		return;

>  	case CEC_MSG_RECORD_STATUS:

>  		return;

> 


Regards,

	Hans
diff mbox series

Patch

diff --git a/utils/cec-compliance/cec-test-power.cpp b/utils/cec-compliance/cec-test-power.cpp
index b675bfc4..c6bf7093 100644
--- a/utils/cec-compliance/cec-test-power.cpp
+++ b/utils/cec-compliance/cec-test-power.cpp
@@ -677,6 +677,67 @@  static int standby_resume_wakeup_deck_play(struct node *node, unsigned me, unsig
 	return standby_resume_wakeup_deck(node, me, la, interactive, CEC_OP_PLAY_MODE_PLAY_FWD);
 }
 
+static int standby_record(struct node *node, unsigned me, unsigned la, bool interactive, __u8 opcode)
+{
+	struct cec_msg msg;
+	__u8 rec_status;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_record_on_own(&msg);
+	msg.reply = CEC_MSG_RECORD_STATUS;
+	fail_on_test(!transmit_timeout(node, &msg, 10000));
+	if (timed_out_or_abort(&msg))
+		return OK_NOT_SUPPORTED;
+	cec_ops_record_status(&msg, &rec_status);
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_CUR_SRC &&
+	             rec_status != CEC_OP_RECORD_STATUS_ALREADY_RECORDING);
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_standby(&msg);
+	fail_on_test(!transmit_timeout(node, &msg));
+	/* Standby should not interrupt the recording. */
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+
+	/* When the recording stops, the recorder should standby unless the recorder is the active source */
+	cec_msg_init(&msg, me, la);
+	if (opcode == CEC_MSG_ACTIVE_SOURCE)
+		cec_msg_active_source(&msg, node->remote[la].phys_addr);
+	else
+		cec_msg_active_source(&msg, node->remote[la].phys_addr + 1);
+	fail_on_test(!transmit_timeout(node, &msg));
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_record_off(&msg, false);
+	fail_on_test(!transmit_timeout(node, &msg));
+
+	unsigned unresponsive_time = 0;
+
+	if (opcode == CEC_MSG_ACTIVE_SOURCE) {
+		fail_on_test(!poll_stable_power_status(node, me, la, CEC_OP_POWER_STATUS_ON, unresponsive_time));
+	} else {
+		fail_on_test(!poll_stable_power_status(node, me, la, CEC_OP_POWER_STATUS_STANDBY, unresponsive_time));
+		fail_on_test(interactive && !question("Is the device in standby?"));
+		node->remote[la].in_standby = true;
+
+		int ret = standby_resume_wakeup(node, me, la, interactive);
+		if (ret)
+			return ret;
+		node->remote[la].in_standby = false;
+	}
+
+	return OK;
+}
+
+static int standby_record_active_source(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	return standby_record(node, me, la, interactive, CEC_MSG_ACTIVE_SOURCE);
+}
+
+static int standby_record_inactive_source(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	return standby_record(node, me, la, interactive, CEC_MSG_INACTIVE_SOURCE);
+}
+
 const vec_remote_subtests standby_resume_subtests{
 	{ "Standby", CEC_LOG_ADDR_MASK_ALL, standby_resume_standby },
 	{ "Repeated Standby message does not wake up", CEC_LOG_ADDR_MASK_ALL, standby_resume_standby_toggle },
@@ -697,4 +758,6 @@  const vec_remote_subtests standby_resume_subtests{
 	{ "Power State Transitions", CEC_LOG_ADDR_MASK_TV, power_state_transitions, false, true },
 	{ "Deck Eject Standby Resume", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, standby_resume_wakeup_deck_eject },
 	{ "Deck Play Standby Resume", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, standby_resume_wakeup_deck_play },
+	{ "Record Standby Active Source", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, standby_record_active_source },
+	{ "Record Standby Inactive Source", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, standby_record_inactive_source },
 };
diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 482192e7..d6c9cdfb 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -318,6 +318,7 @@  void state_init(struct node &node)
 	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
 	node.state.deck_skip_start = 0;
 	node.state.one_touch_record_on = false;
+	node.state.one_touch_record_standby = false;
 	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 3bcb2774..a5d1c3a6 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -54,6 +54,7 @@  struct state {
 	__u8 deck_state;
 	__u64 deck_skip_start;
 	bool one_touch_record_on;
+	bool one_touch_record_standby;
 	time_t toggle_power_status;
 	__u64 last_aud_rate_rx_ts;
 };
@@ -230,5 +231,6 @@  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 void reply_feature_abort(struct node *node, struct cec_msg *msg,
 			 __u8 reason = CEC_OP_ABORT_UNRECOGNIZED_OP);
 void testProcessing(struct node *node, bool wallclock);
+bool enter_standby(struct node *node);
 
 #endif
diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
index f985a841..43e03878 100644
--- a/utils/cec-follower/cec-processing.cpp
+++ b/utils/cec-follower/cec-processing.cpp
@@ -157,7 +157,7 @@  static bool exit_standby(struct node *node)
 	return false;
 }
 
-static bool enter_standby(struct node *node)
+bool enter_standby(struct node *node)
 {
 	if (node->state.power_status == CEC_OP_POWER_STATUS_ON ||
 	    node->state.power_status == CEC_OP_POWER_STATUS_TO_ON) {
@@ -320,10 +320,14 @@  static void processMsg(struct node *node, struct cec_msg &msg, unsigned me, __u8
 		/* Standby */
 
 	case CEC_MSG_STANDBY:
+		/* Standby should not interrupt a recording in progress. */
+		if (node->state.one_touch_record_on) {
+			node->state.one_touch_record_standby = true;
+			break;
+		}
 		enter_standby(node);
 		return;
 
-
 		/* One Touch Play and Routing Control */
 
 	case CEC_MSG_ACTIVE_SOURCE: {
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index 0f40b7d7..4d159456 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -716,6 +716,15 @@  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_TERMINATED_OK);
 		transmit(node, &msg);
 		node->state.one_touch_record_on = false;
+		/*
+		 * If standby was received during recording, enter standby when the recording is finished
+		 * unless the recording device is the currently the active source.
+		 */
+		if (node->state.one_touch_record_standby) {
+			if (node->phys_addr != node->state.active_source_pa)
+				enter_standby(node);
+			node->state.one_touch_record_standby = false;
+		}
 		return;
 	case CEC_MSG_RECORD_STATUS:
 		return;