[PATCH v3 0/3] cec: One Touch Record tests

[PATCH v3 0/3] cec: One Touch Record tests

[Deborah Brouwer]

This is part of an Outreachy project to expand the testing of
One Touch Record messages as handled by CEC adapters.

Changes since v2:
	Patch 1/3 cec: expand One Touch Record TV Screen test:
	- replace numbers with corresponding defines

	Patch 2/3 cec: expand One Touch Record On/Off tests
	- rename commit to reflect expanded scope of tests
	- increase msg timeout for reply to 10s
	- rename helper function and invert return value
	- use primary device type to identify remote follower
	- use logical address type to identify remote initiator
	- limit range of accepted external plug numbers to 6
	- disallow external physical address source
	- keep track of whether the device is recording
	- add additional invalid tests

	Patch 3/3 cec: add One Touch Record Standby tests
	- new patch

Changes since v1:
	Patch 1/2 cec: expand One Touch Record TV Screen test:
	- add space after 'switch'
	- add "return" before fail
	- check analog broadcast type and broadcast system operand
	- add a comment when follower ignores message
	
	Patch 2/2 cec: expand One Touch Record On test
	- new patch

Deborah Brouwer (3):
  cec: expand One Touch Record TV Screen test
  cec: expand One Touch Record On/Off tests
  cec: add One Touch Record Standby tests

 utils/cec-compliance/cec-compliance.h   |   5 +
 utils/cec-compliance/cec-test-power.cpp |  63 +++++++
 utils/cec-compliance/cec-test.cpp       | 211 ++++++++++++++++++++++--
 utils/cec-follower/cec-follower.cpp     |   2 +
 utils/cec-follower/cec-follower.h       |   5 +-
 utils/cec-follower/cec-processing.cpp   |  14 +-
 utils/cec-follower/cec-tuner.cpp        | 119 ++++++++++++-
 7 files changed, 388 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] cec: expand One Touch Record TV Screen test

[Deborah Brouwer]

Check that the follower ignores the Record TV Screen message if the
initiator has a logical address other than Record or Backup (aka Reserved
in CEC Version < 2.0). If the follower replies correctly, check that the
source operand is valid.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 54 ++++++++++++++++++++++++++-----
 utils/cec-follower/cec-tuner.cpp  |  9 ++++--
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 40d8369d..2ea1b712 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -1150,13 +1150,6 @@ static const vec_remote_subtests tuner_ctl_subtests{
 
 static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la, bool interactive)
 {
-	/*
-	  TODO:
-	  - Page 36 in HDMI CEC 1.4b spec lists additional behaviors that should be
-	    checked for.
-	  - The TV should ignore this message when received from other LA than Recording or
-	    Reserved.
-	 */
 	struct cec_msg msg;
 
 	cec_msg_init(&msg, me, la);
@@ -1172,8 +1165,53 @@ static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la,
 		return OK_REFUSED;
 	if (cec_msg_status_is_abort(&msg))
 		return OK_PRESUMED;
+	/*
+	 * Follower should ignore this message if initiator has a logical
+	 * address other than Record or Backup (aka "Reserved" in CEC Version < 2.0).
+	 */
+	if (!cec_has_record(1 << me) && !cec_has_backup(1 << me)) {
+		fail_on_test(!timed_out(&msg));
+		return OK;
+	}
+	fail_on_test(timed_out(&msg));
 
-	return 0;
+	struct cec_op_record_src rec_src = {};
+
+	cec_ops_record_on(&msg, &rec_src);
+
+	if (rec_src.type < CEC_OP_RECORD_SRC_OWN || rec_src.type > CEC_OP_RECORD_SRC_EXT_PHYS_ADDR)
+		return fail("Invalid source.\n");
+
+	if (rec_src.type == CEC_OP_RECORD_SRC_DIGITAL &&
+	    rec_src.digital.service_id_method == CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID) {
+
+		switch (rec_src.digital.dig_bcast_system) {
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T:
+			break;
+		default:
+			return fail("Invalid digital service broadcast system operand.\n");
+		}
+	}
+
+	if (rec_src.type == CEC_OP_RECORD_SRC_ANALOG) {
+		fail_on_test(rec_src.analog.ana_bcast_type > CEC_OP_ANA_BCAST_TYPE_TERRESTRIAL);
+		fail_on_test(rec_src.analog.bcast_system > CEC_OP_BCAST_SYSTEM_PAL_DK &&
+		             rec_src.analog.bcast_system != CEC_OP_BCAST_SYSTEM_OTHER);
+	}
+
+	return OK;
 }
 
 static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool interactive)
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index b9c21684..fd11bd10 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -583,9 +583,6 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		  This is only a basic implementation.
 
 		  TODO:
-		  - If we are a TV, we should only send Record On if the
-		    remote end is a Recording device or Reserved. Otherwise ignore.
-
 		  - Device state should reflect whether we are recording, etc. In
 		    recording mode we should ignore Standby messages.
 		*/
@@ -594,6 +591,12 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		if (!node->has_rec_tv)
 			break;
 
+		__u8 la = cec_msg_initiator(&msg);
+
+		/* Ignore if initiator is not Record or Backup (aka "Reserved" in CEC Version < 2.0) */
+		if (!cec_has_record(1 << la) && !cec_has_backup(1 << la))
+			return;
+
 		struct cec_op_record_src rec_src = {};
 
 		rec_src.type = CEC_OP_RECORD_SRC_OWN;
-- 
2.25.1

Re: [PATCH v3 1/3] cec: expand One Touch Record TV Screen test

[Hans Verkuil]

On 22/06/2021 06:35, Deborah Brouwer wrote:
> Check that the follower ignores the Record TV Screen message if the
> initiator has a logical address other than Record or Backup (aka Reserved
> in CEC Version < 2.0). If the follower replies correctly, check that the
> source operand is valid.
> 
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp | 54 ++++++++++++++++++++++++++-----
>  utils/cec-follower/cec-tuner.cpp  |  9 ++++--
>  2 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 40d8369d..2ea1b712 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -1150,13 +1150,6 @@ static const vec_remote_subtests tuner_ctl_subtests{
>  
>  static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
> -	/*
> -	  TODO:
> -	  - Page 36 in HDMI CEC 1.4b spec lists additional behaviors that should be
> -	    checked for.
> -	  - The TV should ignore this message when received from other LA than Recording or
> -	    Reserved.
> -	 */
>  	struct cec_msg msg;
>  
>  	cec_msg_init(&msg, me, la);
> @@ -1172,8 +1165,53 @@ static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la,
>  		return OK_REFUSED;
>  	if (cec_msg_status_is_abort(&msg))
>  		return OK_PRESUMED;
> +	/*
> +	 * Follower should ignore this message if initiator has a logical
> +	 * address other than Record or Backup (aka "Reserved" in CEC Version < 2.0).
> +	 */
> +	if (!cec_has_record(1 << me) && !cec_has_backup(1 << me)) {
> +		fail_on_test(!timed_out(&msg));
> +		return OK;
> +	}
> +	fail_on_test(timed_out(&msg));
>  
> -	return 0;
> +	struct cec_op_record_src rec_src = {};
> +
> +	cec_ops_record_on(&msg, &rec_src);
> +
> +	if (rec_src.type < CEC_OP_RECORD_SRC_OWN || rec_src.type > CEC_OP_RECORD_SRC_EXT_PHYS_ADDR)
> +		return fail("Invalid source.\n");
> +
> +	if (rec_src.type == CEC_OP_RECORD_SRC_DIGITAL &&
> +	    rec_src.digital.service_id_method == CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID) {
> +

No need for this empty line.

> +		switch (rec_src.digital.dig_bcast_system) {
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T:
> +			break;
> +		default:
> +			return fail("Invalid digital service broadcast system operand.\n");
> +		}
> +	}
> +
> +	if (rec_src.type == CEC_OP_RECORD_SRC_ANALOG) {
> +		fail_on_test(rec_src.analog.ana_bcast_type > CEC_OP_ANA_BCAST_TYPE_TERRESTRIAL);
> +		fail_on_test(rec_src.analog.bcast_system > CEC_OP_BCAST_SYSTEM_PAL_DK &&
> +		             rec_src.analog.bcast_system != CEC_OP_BCAST_SYSTEM_OTHER);

You can also check the analog frequency here: corner cases 0x0000 and 0xffff are invalid.

> +	}

For CEC_OP_RECORD_SRC_EXT_PLUG you can check against the invalid value 0x00.

Regards,

	Hans

> +
> +	return OK;
>  }
>  
>  static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool interactive)
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index b9c21684..fd11bd10 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -583,9 +583,6 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		  This is only a basic implementation.
>  
>  		  TODO:
> -		  - If we are a TV, we should only send Record On if the
> -		    remote end is a Recording device or Reserved. Otherwise ignore.
> -
>  		  - Device state should reflect whether we are recording, etc. In
>  		    recording mode we should ignore Standby messages.
>  		*/
> @@ -594,6 +591,12 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		if (!node->has_rec_tv)
>  			break;
>  
> +		__u8 la = cec_msg_initiator(&msg);
> +
> +		/* Ignore if initiator is not Record or Backup (aka "Reserved" in CEC Version < 2.0) */
> +		if (!cec_has_record(1 << la) && !cec_has_backup(1 << la))
> +			return;
> +
>  		struct cec_op_record_src rec_src = {};
>  
>  		rec_src.type = CEC_OP_RECORD_SRC_OWN;
> 

[PATCH v3 2/3] cec: expand One Touch Record On/Off tests

[Deborah Brouwer]

Send all Record On source operands and check that the follower responds
with a corresponding Record Status. Send invalid Record On operands and
check that the follower returns Feature Abort with Invalid Operand or
Record Status indicating why the recording has failed.

Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
---
 utils/cec-compliance/cec-compliance.h |   5 +
 utils/cec-compliance/cec-test.cpp     | 157 ++++++++++++++++++++++++--
 utils/cec-follower/cec-follower.cpp   |   1 +
 utils/cec-follower/cec-follower.h     |   3 +-
 utils/cec-follower/cec-processing.cpp |   7 +-
 utils/cec-follower/cec-tuner.cpp      | 101 ++++++++++++++++-
 6 files changed, 255 insertions(+), 19 deletions(-)

diff --git a/utils/cec-compliance/cec-compliance.h b/utils/cec-compliance/cec-compliance.h
index 818181ab..d6c11d70 100644
--- a/utils/cec-compliance/cec-compliance.h
+++ b/utils/cec-compliance/cec-compliance.h
@@ -331,6 +331,11 @@ static inline bool is_playback_or_rec(unsigned la)
 	return cec_has_playback(1 << la) || cec_has_record(1 << la);
 }
 
+static inline bool is_record(unsigned la, unsigned prim_type)
+{
+	return cec_has_record(1 << la) || (cec_has_backup(1 << la) && prim_type == CEC_OP_PRIM_DEVTYPE_RECORD);
+}
+
 static inline bool cec_msg_status_is_abort(const struct cec_msg *msg)
 {
 	return msg->rx_status & CEC_RX_STATUS_FEATURE_ABORT;
diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 2ea1b712..4712f993 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -48,6 +48,45 @@ static int test_play_mode(struct node *node, unsigned me, unsigned la, __u8 play
 	return OK;
 }
 
+static int one_touch_rec_on_send(struct node *node, unsigned me, unsigned la, const struct cec_op_record_src &rec_src, __u8 &rec_status)
+{
+	struct cec_msg msg;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_record_off(&msg, false);
+	fail_on_test(!transmit_timeout(node, &msg));
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_record_on(&msg, true, &rec_src);
+	fail_on_test(!transmit_timeout(node, &msg, 10000));
+	fail_on_test(timed_out_or_abort(&msg));
+	cec_ops_record_status(&msg, &rec_status);
+
+	return OK;
+}
+
+static bool one_touch_rec_status_not_shared(__u8 rec_status)
+{
+	switch (rec_status) {
+	case CEC_OP_RECORD_STATUS_UNSUP_CA:
+	case CEC_OP_RECORD_STATUS_NO_CA_ENTITLEMENTS:
+	case CEC_OP_RECORD_STATUS_CANT_COPY_SRC:
+	case CEC_OP_RECORD_STATUS_NO_MORE_COPIES:
+	case CEC_OP_RECORD_STATUS_NO_MEDIA:
+	case CEC_OP_RECORD_STATUS_PLAYING:
+	case CEC_OP_RECORD_STATUS_ALREADY_RECORDING:
+	case CEC_OP_RECORD_STATUS_MEDIA_PROT:
+	case CEC_OP_RECORD_STATUS_NO_SIGNAL:
+	case CEC_OP_RECORD_STATUS_MEDIA_PROBLEM:
+	case CEC_OP_RECORD_STATUS_NO_SPACE:
+	case CEC_OP_RECORD_STATUS_PARENTAL_LOCK:
+	case CEC_OP_RECORD_STATUS_OTHER:
+		return false;
+	default:
+		return true;
+	}
+}
+
 /* System Information */
 
 int system_info_polling(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -1216,27 +1255,116 @@ static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la,
 
 static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool interactive)
 {
-	/*
-	  TODO: Page 36 in HDMI CEC 1.4b spec lists additional behaviors that should be
-	  checked for.
-	 */
 	struct cec_msg msg;
 	struct cec_op_record_src rec_src = {};
 
 	rec_src.type = CEC_OP_RECORD_SRC_OWN;
 	cec_msg_init(&msg, me, la);
 	cec_msg_record_on(&msg, true, &rec_src);
-	fail_on_test(!transmit_timeout(node, &msg));
+	/* Allow 10s for reply because specs say it may take several seconds to accurately respond. */
+	fail_on_test(!transmit_timeout(node, &msg, 10000));
 	fail_on_test(timed_out(&msg));
-	fail_on_test(cec_has_record(1 << la) && unrecognized_op(&msg));
-	if (unrecognized_op(&msg))
+	if (unrecognized_op(&msg)) {
+		fail_on_test(is_record(la, node->remote[la].prim_type));
 		return OK_NOT_SUPPORTED;
+	}
 	if (refused(&msg))
 		return OK_REFUSED;
 	if (cec_msg_status_is_abort(&msg))
 		return OK_PRESUMED;
 
-	return 0;
+	__u8 rec_status;
+
+	cec_ops_record_status(&msg, &rec_status);
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_CUR_SRC &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	rec_src.type = CEC_OP_RECORD_SRC_DIGITAL;
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_DIG_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_DIG_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	rec_src.type = CEC_OP_RECORD_SRC_ANALOG;
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_ANA_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	rec_src.type = CEC_OP_RECORD_SRC_EXT_PLUG;
+	rec_src.ext_plug.plug = 1;
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	memset(&rec_src, 0, sizeof(rec_src));
+	rec_src.type = CEC_OP_RECORD_SRC_EXT_PHYS_ADDR;
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
+	             rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PHYS_ADDR &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	return OK;
+}
+
+static int one_touch_rec_on_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_record_on_own(&msg);
+	msg.msg[2] = 0;  /* Invalid source operand */
+	fail_on_test(!transmit_timeout(node, &msg, 10000));
+	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_record_on_own(&msg);
+	msg.msg[2] = 6;  /* Invalid source operand */
+	fail_on_test(!transmit_timeout(node, &msg, 10000));
+	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);
+
+	struct cec_op_record_src rec_src = {};
+	__u8 rec_status;
+
+	rec_src.type = CEC_OP_RECORD_SRC_DIGITAL;
+	rec_src.digital.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID;
+	rec_src.digital.dig_bcast_system = 3; /* Invalid digital broadcast system */
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_DIG_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	memset(&rec_src, 0, sizeof(rec_src));
+	rec_src.type = CEC_OP_RECORD_SRC_ANALOG;
+	rec_src.analog.ana_bcast_type = 3; /* Invalid analog broadcast type */
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	rec_src.analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
+	rec_src.analog.bcast_system = 9; /* Invalid analog broadcast system */
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
+	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	memset(&rec_src, 0, sizeof(rec_src));
+	rec_src.type = CEC_OP_RECORD_SRC_EXT_PLUG;
+	rec_src.ext_plug.plug = 0; /* Invalid plug */
+	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG &&
+	             one_touch_rec_status_not_shared(rec_status));
+
+	return OK;
 }
 
 static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -1261,8 +1389,17 @@ static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool i
 
 static const vec_remote_subtests one_touch_rec_subtests{
 	{ "Record TV Screen", CEC_LOG_ADDR_MASK_TV, one_touch_rec_tv_screen },
-	{ "Record On", CEC_LOG_ADDR_MASK_RECORD, one_touch_rec_on },
-	{ "Record Off", CEC_LOG_ADDR_MASK_RECORD, one_touch_rec_off },
+	{
+		"Record On",
+		CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP,
+		one_touch_rec_on,
+	},
+	{
+		"Record On Invalid Operand",
+		CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP,
+		one_touch_rec_on_invalid,
+	},
+	{ "Record Off", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, one_touch_rec_off },
 };
 
 /* Timer Programming */
diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index ff47d698..482192e7 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -317,6 +317,7 @@ void state_init(struct node &node)
 	node.state.deck_report_changes_to = 0;
 	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
 	node.state.deck_skip_start = 0;
+	node.state.one_touch_record_on = 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 3fa95417..8e8877d7 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -53,6 +53,7 @@ struct state {
 	__u8 deck_report_changes_to;
 	__u8 deck_state;
 	__u64 deck_skip_start;
+	bool one_touch_record_on;
 	time_t toggle_power_status;
 	__u64 last_aud_rate_rx_ts;
 };
@@ -222,7 +223,7 @@ void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
 
 // cec-tuner.cpp
 void tuner_dev_info_init(struct state *state);
-void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
+void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me, __u8 type);
 
 // CEC processing
 void reply_feature_abort(struct node *node, struct cec_msg *msg,
diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
index 41bb990c..b1c8f3d9 100644
--- a/utils/cec-follower/cec-processing.cpp
+++ b/utils/cec-follower/cec-processing.cpp
@@ -271,7 +271,7 @@ static void update_deck_state(struct node *node, unsigned me, __u8 deck_state_ne
 	}
 }
 
-static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
+static void processMsg(struct node *node, struct cec_msg &msg, unsigned me, __u8 type)
 {
 	__u8 to = cec_msg_destination(&msg);
 	__u8 from = cec_msg_initiator(&msg);
@@ -672,7 +672,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
 	case CEC_MSG_SET_TIMER_PROGRAM_TITLE:
 	case CEC_MSG_TIMER_CLEARED_STATUS:
 	case CEC_MSG_TIMER_STATUS:
-		process_tuner_record_timer_msgs(node, msg, me);
+		process_tuner_record_timer_msgs(node, msg, me, type);
 		return;
 
 		/* Dynamic Auto Lipsync */
@@ -1009,6 +1009,7 @@ void testProcessing(struct node *node, bool wallclock)
 	doioctl(node, CEC_S_MODE, &mode);
 	doioctl(node, CEC_ADAP_G_LOG_ADDRS, &laddrs);
 	me = laddrs.log_addr[0];
+	__u8 type = laddrs.log_addr_type[0];
 
 	poll_remote_devs(node, me);
 
@@ -1088,7 +1089,7 @@ void testProcessing(struct node *node, bool wallclock)
 					       msg.sequence, ts2s(msg.rx_ts, wallclock).c_str());
 			}
 			if (node->adap_la_mask)
-				processMsg(node, msg, me);
+				processMsg(node, msg, me, type);
 		}
 
 		__u8 pwr_state = current_power_state(node);
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index fd11bd10..c6dd47f0 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -482,7 +482,47 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	return false;
 }
 
-void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
+bool digital_service_info_valid(const struct cec_op_record_src &rec_src)
+{
+	if (rec_src.digital.service_id_method == CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID) {
+
+		switch (rec_src.digital.dig_bcast_system) {
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2:
+		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T:
+			break;
+		default:
+			return false;
+		}
+	}
+	//To do: check digital_arib_data, digital_atsc_data and digital_dvb_data
+	return true;
+}
+
+bool analog_service_info_valid(const cec_op_record_src &rec_src)
+{
+	if (rec_src.analog.ana_bcast_type > CEC_OP_ANA_BCAST_TYPE_TERRESTRIAL)
+		return false;
+
+	if(rec_src.analog.bcast_system > CEC_OP_BCAST_SYSTEM_PAL_DK &&
+	   rec_src.analog.bcast_system != CEC_OP_BCAST_SYSTEM_OTHER)
+		return false;
+
+	//To do: check analog valid channels analog_freqs_khz
+	return true;
+}
+
+void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me, __u8 type)
 {
 	bool is_bcast = cec_msg_is_broadcast(&msg);
 
@@ -605,19 +645,70 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		transmit(node, &msg);
 		return;
 	}
-	case CEC_MSG_RECORD_ON:
-		if (!cec_has_record(1 << me))
+	case CEC_MSG_RECORD_ON: {
+		if (type != CEC_LOG_ADDR_TYPE_RECORD)
+			break;
+
+		__u8 rec_status;
+		bool start_recording = false;
+		struct cec_op_record_src rec_src = {};
+
+		cec_ops_record_on(&msg, &rec_src);
+
+		switch (rec_src.type) {
+		case CEC_OP_RECORD_SRC_OWN:
+			rec_status = CEC_OP_RECORD_STATUS_CUR_SRC;
+			start_recording = true;
+			break;
+		case CEC_OP_RECORD_SRC_DIGITAL:
+			if (digital_service_info_valid(rec_src)) {
+				rec_status = CEC_OP_RECORD_STATUS_DIG_SERVICE;
+				start_recording = true;
+			} else {
+				rec_status = CEC_OP_RECORD_STATUS_NO_DIG_SERVICE;
+			}
+			break;
+		case CEC_OP_RECORD_SRC_ANALOG:
+			if (analog_service_info_valid(rec_src)) {
+				rec_status = CEC_OP_RECORD_STATUS_ANA_SERVICE;
+				start_recording = true;
+			} else {
+				rec_status = CEC_OP_RECORD_STATUS_NO_ANA_SERVICE;
+			}
+			break;
+		case CEC_OP_RECORD_SRC_EXT_PLUG:
+			/* Plug number range is 1-255 in specs, but a realistic range of connectors is 6. */
+			if (rec_src.ext_plug.plug < 1 || rec_src.ext_plug.plug > 6) {
+				rec_status = CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG;
+			} else {
+				rec_status = CEC_OP_RECORD_STATUS_EXT_INPUT;
+				start_recording = true;
+			}
+			break;
+		case CEC_OP_RECORD_SRC_EXT_PHYS_ADDR:
+			rec_status = CEC_OP_RECORD_STATUS_INVALID_EXT_PHYS_ADDR;
 			break;
+		default:
+			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
+			return;
+		}
+		if (node->state.one_touch_record_on)
+			rec_status = CEC_OP_RECORD_STATUS_ALREADY_RECORDING;
 		cec_msg_set_reply_to(&msg, &msg);
-		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_CUR_SRC);
+		cec_msg_record_status(&msg, rec_status);
 		transmit(node, &msg);
+		if (start_recording)
+			node->state.one_touch_record_on = true;
 		return;
+	}
 	case CEC_MSG_RECORD_OFF:
-		if (!cec_has_record(1 << me))
+		if (type != CEC_LOG_ADDR_TYPE_RECORD)
 			break;
+
 		cec_msg_set_reply_to(&msg, &msg);
 		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_TERMINATED_OK);
 		transmit(node, &msg);
+		node->state.one_touch_record_on = false;
 		return;
 	case CEC_MSG_RECORD_STATUS:
 		return;
-- 
2.25.1

Re: [PATCH v3 2/3] cec: expand One Touch Record On/Off tests

[Hans Verkuil]

On 22/06/2021 06:36, Deborah Brouwer wrote:
> Send all Record On source operands and check that the follower responds
> with a corresponding Record Status. Send invalid Record On operands and
> check that the follower returns Feature Abort with Invalid Operand or
> Record Status indicating why the recording has failed.
> 
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
> ---
>  utils/cec-compliance/cec-compliance.h |   5 +
>  utils/cec-compliance/cec-test.cpp     | 157 ++++++++++++++++++++++++--
>  utils/cec-follower/cec-follower.cpp   |   1 +
>  utils/cec-follower/cec-follower.h     |   3 +-
>  utils/cec-follower/cec-processing.cpp |   7 +-
>  utils/cec-follower/cec-tuner.cpp      | 101 ++++++++++++++++-
>  6 files changed, 255 insertions(+), 19 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-compliance.h b/utils/cec-compliance/cec-compliance.h
> index 818181ab..d6c11d70 100644
> --- a/utils/cec-compliance/cec-compliance.h
> +++ b/utils/cec-compliance/cec-compliance.h
> @@ -331,6 +331,11 @@ static inline bool is_playback_or_rec(unsigned la)
>  	return cec_has_playback(1 << la) || cec_has_record(1 << la);
>  }
>  
> +static inline bool is_record(unsigned la, unsigned prim_type)
> +{
> +	return cec_has_record(1 << la) || (cec_has_backup(1 << la) && prim_type == CEC_OP_PRIM_DEVTYPE_RECORD);
> +}
> +
>  static inline bool cec_msg_status_is_abort(const struct cec_msg *msg)
>  {
>  	return msg->rx_status & CEC_RX_STATUS_FEATURE_ABORT;
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 2ea1b712..4712f993 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -48,6 +48,45 @@ static int test_play_mode(struct node *node, unsigned me, unsigned la, __u8 play
>  	return OK;
>  }
>  
> +static int one_touch_rec_on_send(struct node *node, unsigned me, unsigned la, const struct cec_op_record_src &rec_src, __u8 &rec_status)

This line is too long, add a line break after the la argument.

> +{
> +	struct cec_msg msg;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_record_off(&msg, false);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_record_on(&msg, true, &rec_src);
> +	fail_on_test(!transmit_timeout(node, &msg, 10000));
> +	fail_on_test(timed_out_or_abort(&msg));
> +	cec_ops_record_status(&msg, &rec_status);
> +
> +	return OK;
> +}
> +
> +static bool one_touch_rec_status_not_shared(__u8 rec_status)
> +{
> +	switch (rec_status) {
> +	case CEC_OP_RECORD_STATUS_UNSUP_CA:
> +	case CEC_OP_RECORD_STATUS_NO_CA_ENTITLEMENTS:
> +	case CEC_OP_RECORD_STATUS_CANT_COPY_SRC:
> +	case CEC_OP_RECORD_STATUS_NO_MORE_COPIES:
> +	case CEC_OP_RECORD_STATUS_NO_MEDIA:
> +	case CEC_OP_RECORD_STATUS_PLAYING:
> +	case CEC_OP_RECORD_STATUS_ALREADY_RECORDING:
> +	case CEC_OP_RECORD_STATUS_MEDIA_PROT:
> +	case CEC_OP_RECORD_STATUS_NO_SIGNAL:
> +	case CEC_OP_RECORD_STATUS_MEDIA_PROBLEM:
> +	case CEC_OP_RECORD_STATUS_NO_SPACE:
> +	case CEC_OP_RECORD_STATUS_PARENTAL_LOCK:
> +	case CEC_OP_RECORD_STATUS_OTHER:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
>  /* System Information */
>  
>  int system_info_polling(struct node *node, unsigned me, unsigned la, bool interactive)
> @@ -1216,27 +1255,116 @@ static int one_touch_rec_tv_screen(struct node *node, unsigned me, unsigned la,
>  
>  static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
> -	/*
> -	  TODO: Page 36 in HDMI CEC 1.4b spec lists additional behaviors that should be
> -	  checked for.
> -	 */
>  	struct cec_msg msg;
>  	struct cec_op_record_src rec_src = {};
>  
>  	rec_src.type = CEC_OP_RECORD_SRC_OWN;
>  	cec_msg_init(&msg, me, la);
>  	cec_msg_record_on(&msg, true, &rec_src);
> -	fail_on_test(!transmit_timeout(node, &msg));
> +	/* Allow 10s for reply because specs say it may take several seconds to accurately respond. */

specs -> the spec

There is only on spec, so singular.

> +	fail_on_test(!transmit_timeout(node, &msg, 10000));
>  	fail_on_test(timed_out(&msg));
> -	fail_on_test(cec_has_record(1 << la) && unrecognized_op(&msg));
> -	if (unrecognized_op(&msg))
> +	if (unrecognized_op(&msg)) {
> +		fail_on_test(is_record(la, node->remote[la].prim_type));
>  		return OK_NOT_SUPPORTED;
> +	}
>  	if (refused(&msg))
>  		return OK_REFUSED;
>  	if (cec_msg_status_is_abort(&msg))
>  		return OK_PRESUMED;
>  
> -	return 0;
> +	__u8 rec_status;
> +
> +	cec_ops_record_status(&msg, &rec_status);
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_CUR_SRC &&
> +	             one_touch_rec_status_not_shared(rec_status));

As discussed in irc I suggest something like this:

	if (rec_status != CEC_OP_RECORD_STATUS_CUR_SRC)
		fail_on_test(!rec_status_is_a_valid_error_status(rec_status));

I.e., if it did not start recording with the current source, then check if
the status is a valid error, otherwise something strange happened and that should
be marked as a failure.

> +
> +	rec_src.type = CEC_OP_RECORD_SRC_DIGITAL;
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_DIG_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_DIG_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&

I wouldn't test for these error cases, just add that to rec_status_is_a_valid_error_status().

> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	rec_src.type = CEC_OP_RECORD_SRC_ANALOG;
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_ANA_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&

Ditto.

> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	rec_src.type = CEC_OP_RECORD_SRC_EXT_PLUG;
> +	rec_src.ext_plug.plug = 1;
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	memset(&rec_src, 0, sizeof(rec_src));
> +	rec_src.type = CEC_OP_RECORD_SRC_EXT_PHYS_ADDR;
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
> +	             rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PHYS_ADDR &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	return OK;
> +}
> +
> +static int one_touch_rec_on_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> +	struct cec_msg msg;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_record_on_own(&msg);
> +	msg.msg[2] = 0;  /* Invalid source operand */
> +	fail_on_test(!transmit_timeout(node, &msg, 10000));

Reacting to invalid operands shouldn't take 10 seconds, just use the default timeout.
Same below.

> +	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_record_on_own(&msg);
> +	msg.msg[2] = 6;  /* Invalid source operand */
> +	fail_on_test(!transmit_timeout(node, &msg, 10000));
> +	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);
> +
> +	struct cec_op_record_src rec_src = {};
> +	__u8 rec_status;
> +
> +	rec_src.type = CEC_OP_RECORD_SRC_DIGITAL;
> +	rec_src.digital.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID;
> +	rec_src.digital.dig_bcast_system = 3; /* Invalid digital broadcast system */
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));

Shouldn't this feature abort with invalid op? This looks weird. Same below.

> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_DIG_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	memset(&rec_src, 0, sizeof(rec_src));
> +	rec_src.type = CEC_OP_RECORD_SRC_ANALOG;
> +	rec_src.analog.ana_bcast_type = 3; /* Invalid analog broadcast type */
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	rec_src.analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
> +	rec_src.analog.bcast_system = 9; /* Invalid analog broadcast system */
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_NO_ANA_SERVICE &&
> +	             rec_status != CEC_OP_RECORD_STATUS_NO_SERVICE &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	memset(&rec_src, 0, sizeof(rec_src));
> +	rec_src.type = CEC_OP_RECORD_SRC_EXT_PLUG;
> +	rec_src.ext_plug.plug = 0; /* Invalid plug */
> +	fail_on_test(one_touch_rec_on_send(node, me, la, rec_src, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG &&
> +	             one_touch_rec_status_not_shared(rec_status));
> +
> +	return OK;
>  }
>  
>  static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool interactive)
> @@ -1261,8 +1389,17 @@ static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool i
>  
>  static const vec_remote_subtests one_touch_rec_subtests{
>  	{ "Record TV Screen", CEC_LOG_ADDR_MASK_TV, one_touch_rec_tv_screen },
> -	{ "Record On", CEC_LOG_ADDR_MASK_RECORD, one_touch_rec_on },
> -	{ "Record Off", CEC_LOG_ADDR_MASK_RECORD, one_touch_rec_off },
> +	{
> +		"Record On",
> +		CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP,
> +		one_touch_rec_on,
> +	},
> +	{
> +		"Record On Invalid Operand",
> +		CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP,
> +		one_touch_rec_on_invalid,
> +	},
> +	{ "Record Off", CEC_LOG_ADDR_MASK_RECORD | CEC_LOG_ADDR_MASK_BACKUP, one_touch_rec_off },
>  };
>  
>  /* Timer Programming */
> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
> index ff47d698..482192e7 100644
> --- a/utils/cec-follower/cec-follower.cpp
> +++ b/utils/cec-follower/cec-follower.cpp
> @@ -317,6 +317,7 @@ void state_init(struct node &node)
>  	node.state.deck_report_changes_to = 0;
>  	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
>  	node.state.deck_skip_start = 0;
> +	node.state.one_touch_record_on = 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 3fa95417..8e8877d7 100644
> --- a/utils/cec-follower/cec-follower.h
> +++ b/utils/cec-follower/cec-follower.h
> @@ -53,6 +53,7 @@ struct state {
>  	__u8 deck_report_changes_to;
>  	__u8 deck_state;
>  	__u64 deck_skip_start;
> +	bool one_touch_record_on;
>  	time_t toggle_power_status;
>  	__u64 last_aud_rate_rx_ts;
>  };
> @@ -222,7 +223,7 @@ void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
>  
>  // cec-tuner.cpp
>  void tuner_dev_info_init(struct state *state);
> -void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
> +void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me, __u8 type);
>  
>  // CEC processing
>  void reply_feature_abort(struct node *node, struct cec_msg *msg,
> diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
> index 41bb990c..b1c8f3d9 100644
> --- a/utils/cec-follower/cec-processing.cpp
> +++ b/utils/cec-follower/cec-processing.cpp
> @@ -271,7 +271,7 @@ static void update_deck_state(struct node *node, unsigned me, __u8 deck_state_ne
>  	}
>  }
>  
> -static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
> +static void processMsg(struct node *node, struct cec_msg &msg, unsigned me, __u8 type)

Adding the type argument and wiring it up to process_tuner_record_timer_msgs() should
be done in a separate patch. It's not related to the record tests as such.

>  {
>  	__u8 to = cec_msg_destination(&msg);
>  	__u8 from = cec_msg_initiator(&msg);
> @@ -672,7 +672,7 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
>  	case CEC_MSG_SET_TIMER_PROGRAM_TITLE:
>  	case CEC_MSG_TIMER_CLEARED_STATUS:
>  	case CEC_MSG_TIMER_STATUS:
> -		process_tuner_record_timer_msgs(node, msg, me);
> +		process_tuner_record_timer_msgs(node, msg, me, type);
>  		return;
>  
>  		/* Dynamic Auto Lipsync */
> @@ -1009,6 +1009,7 @@ void testProcessing(struct node *node, bool wallclock)
>  	doioctl(node, CEC_S_MODE, &mode);
>  	doioctl(node, CEC_ADAP_G_LOG_ADDRS, &laddrs);
>  	me = laddrs.log_addr[0];
> +	__u8 type = laddrs.log_addr_type[0];
>  
>  	poll_remote_devs(node, me);
>  
> @@ -1088,7 +1089,7 @@ void testProcessing(struct node *node, bool wallclock)
>  					       msg.sequence, ts2s(msg.rx_ts, wallclock).c_str());
>  			}
>  			if (node->adap_la_mask)
> -				processMsg(node, msg, me);
> +				processMsg(node, msg, me, type);
>  		}
>  
>  		__u8 pwr_state = current_power_state(node);
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index fd11bd10..c6dd47f0 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -482,7 +482,47 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	return false;
>  }
>  
> -void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
> +bool digital_service_info_valid(const struct cec_op_record_src &rec_src)

Can this be a static function? It's only used in this source, so I think t should be.
Ditto for the similar bool functions below.

> +{
> +	if (rec_src.digital.service_id_method == CEC_OP_SERVICE_ID_METHOD_BY_DIG_ID) {
> +

Drop this newline.

> +		switch (rec_src.digital.dig_bcast_system) {
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2:
> +		case CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T:
> +			break;
> +		default:
> +			return false;
> +		}
> +	}
> +	//To do: check digital_arib_data, digital_atsc_data and digital_dvb_data
> +	return true;
> +}
> +
> +bool analog_service_info_valid(const cec_op_record_src &rec_src)
> +{
> +	if (rec_src.analog.ana_bcast_type > CEC_OP_ANA_BCAST_TYPE_TERRESTRIAL)
> +		return false;
> +
> +	if(rec_src.analog.bcast_system > CEC_OP_BCAST_SYSTEM_PAL_DK &&
> +	   rec_src.analog.bcast_system != CEC_OP_BCAST_SYSTEM_OTHER)
> +		return false;
> +
> +	//To do: check analog valid channels analog_freqs_khz
> +	return true;
> +}
> +
> +void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me, __u8 type)
>  {
>  	bool is_bcast = cec_msg_is_broadcast(&msg);
>  
> @@ -605,19 +645,70 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		transmit(node, &msg);
>  		return;
>  	}
> -	case CEC_MSG_RECORD_ON:
> -		if (!cec_has_record(1 << me))
> +	case CEC_MSG_RECORD_ON: {
> +		if (type != CEC_LOG_ADDR_TYPE_RECORD)
> +			break;
> +
> +		__u8 rec_status;
> +		bool start_recording = false;
> +		struct cec_op_record_src rec_src = {};
> +
> +		cec_ops_record_on(&msg, &rec_src);
> +
> +		switch (rec_src.type) {
> +		case CEC_OP_RECORD_SRC_OWN:
> +			rec_status = CEC_OP_RECORD_STATUS_CUR_SRC;
> +			start_recording = true;
> +			break;
> +		case CEC_OP_RECORD_SRC_DIGITAL:
> +			if (digital_service_info_valid(rec_src)) {
> +				rec_status = CEC_OP_RECORD_STATUS_DIG_SERVICE;
> +				start_recording = true;
> +			} else {
> +				rec_status = CEC_OP_RECORD_STATUS_NO_DIG_SERVICE;

Huh? Shouldn't this feature abort with invalid op? Since you provide invalid data here.

> +			}
> +			break;
> +		case CEC_OP_RECORD_SRC_ANALOG:
> +			if (analog_service_info_valid(rec_src)) {
> +				rec_status = CEC_OP_RECORD_STATUS_ANA_SERVICE;
> +				start_recording = true;
> +			} else {
> +				rec_status = CEC_OP_RECORD_STATUS_NO_ANA_SERVICE;
> +			}
> +			break;
> +		case CEC_OP_RECORD_SRC_EXT_PLUG:
> +			/* Plug number range is 1-255 in specs, but a realistic range of connectors is 6. */
> +			if (rec_src.ext_plug.plug < 1 || rec_src.ext_plug.plug > 6) {
> +				rec_status = CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG;
> +			} else {
> +				rec_status = CEC_OP_RECORD_STATUS_EXT_INPUT;
> +				start_recording = true;
> +			}
> +			break;
> +		case CEC_OP_RECORD_SRC_EXT_PHYS_ADDR:
> +			rec_status = CEC_OP_RECORD_STATUS_INVALID_EXT_PHYS_ADDR;
>  			break;
> +		default:
> +			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
> +			return;
> +		}
> +		if (node->state.one_touch_record_on)
> +			rec_status = CEC_OP_RECORD_STATUS_ALREADY_RECORDING;
>  		cec_msg_set_reply_to(&msg, &msg);
> -		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_CUR_SRC);
> +		cec_msg_record_status(&msg, rec_status);
>  		transmit(node, &msg);
> +		if (start_recording)
> +			node->state.one_touch_record_on = true;
>  		return;
> +	}
>  	case CEC_MSG_RECORD_OFF:
> -		if (!cec_has_record(1 << me))
> +		if (type != CEC_LOG_ADDR_TYPE_RECORD)
>  			break;
> +
>  		cec_msg_set_reply_to(&msg, &msg);
>  		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_TERMINATED_OK);
>  		transmit(node, &msg);
> +		node->state.one_touch_record_on = false;
>  		return;
>  	case CEC_MSG_RECORD_STATUS:
>  		return;
> 

Regards,

	Hans

[PATCH v3 3/3] cec: add One Touch Record Standby tests

[Deborah Brouwer]

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   |  7 ++-
 utils/cec-follower/cec-tuner.cpp        |  9 ++++
 5 files changed, 81 insertions(+), 1 deletion(-)

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..ff8bdff7 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -317,6 +317,7 @@ void state_init(struct node &node)
 	node.state.deck_report_changes_to = 0;
 	node.state.deck_state = CEC_OP_DECK_INFO_STOP;
 	node.state.deck_skip_start = 0;
+	node.state.one_touch_record_standby = false;
 	node.state.one_touch_record_on = 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 8e8877d7..4fdc4aec 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -53,6 +53,7 @@ struct state {
 	__u8 deck_report_changes_to;
 	__u8 deck_state;
 	__u64 deck_skip_start;
+	bool one_touch_record_standby;
 	bool one_touch_record_on;
 	time_t toggle_power_status;
 	__u64 last_aud_rate_rx_ts;
@@ -229,5 +230,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 b1c8f3d9..b4ddb8b4 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,6 +320,11 @@ 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;
 
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index c6dd47f0..225fa934 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -708,6 +708,15 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		cec_msg_set_reply_to(&msg, &msg);
 		cec_msg_record_status(&msg, CEC_OP_RECORD_STATUS_TERMINATED_OK);
 		transmit(node, &msg);
+		/*
+		 * 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;
+		}
 		node->state.one_touch_record_on = false;
 		return;
 	case CEC_MSG_RECORD_STATUS:
-- 
2.25.1