[PATCH v2 0/2] cec: One Touch Record tests

[PATCH v2 0/2] 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 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 (2):
  cec: expand One Touch Record TV Screen test
  cec: expand One Touch Record On test

 utils/cec-compliance/cec-test.cpp | 160 +++++++++++++++++++++++++++---
 utils/cec-follower/cec-tuner.cpp  |  37 ++++++-
 2 files changed, 176 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] 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 | 53 ++++++++++++++++++++++++++-----
 utils/cec-follower/cec-tuner.cpp  |  9 ++++--
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 40d8369d..0051d72a 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,52 @@ 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 < 1 || rec_src.type > 5)
+		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 > 2);
+		fail_on_test(rec_src.analog.bcast_system > 8 && rec_src.analog.bcast_system != 0x1f);
+	}
+
+	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

[PATCH v2 2/2] cec: expand One Touch Record On test

[Deborah Brouwer]

Send all Record On source operands and check that the follower responds
with a corresponding Record Status. Send invalid Record On source
operands and make sure that the follower returns Feature Abort with
Invalid Operand.

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

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 0051d72a..2c64d1a0 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -48,6 +48,43 @@ 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, __u8 src, __u8 &rec_status)
+{
+	struct cec_msg msg;
+	struct cec_op_record_src rec_src = {};
+
+	cec_msg_init(&msg, me, la);
+	rec_src.type = src;
+	cec_msg_record_on(&msg, true, &rec_src);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(timed_out_or_abort(&msg));
+	cec_ops_record_status(&msg, &rec_status);
+
+	return OK;
+}
+
+static bool one_touch_rec_common(__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 true;
+	default:
+		return false;
+	}
+}
+
 /* System Information */
 
 int system_info_polling(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -1215,10 +1252,6 @@ 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 = {};
 
@@ -1227,15 +1260,68 @@ static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool in
 	cec_msg_record_on(&msg, true, &rec_src);
 	fail_on_test(!transmit_timeout(node, &msg));
 	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(cec_has_record(1 << la) || cec_has_backup(1 << la));
 		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_common(rec_status));
+
+	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_DIGITAL, 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_common(rec_status));
+
+	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_ANALOG, 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_common(rec_status));
+
+	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_EXT_PLUG, rec_status));
+	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
+	             rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG &&
+	             !one_touch_rec_common(rec_status));
+
+	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_EXT_PHYS_ADDR, 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_common(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));
+	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));
+	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);
+
+	return OK;
 }
 
 static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool interactive)
@@ -1260,7 +1346,12 @@ 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 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, one_touch_rec_off },
 };
 
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index fd11bd10..ebceb064 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -606,10 +606,34 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 	}
 	case CEC_MSG_RECORD_ON:
-		if (!cec_has_record(1 << me))
+		if (!cec_has_record(1 << me) && !cec_has_backup(1 << me))
+			break;
+
+		struct cec_op_record_src rec_src;
+		__u8 rec_status;
+
+		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;
+			break;
+		case CEC_OP_RECORD_SRC_DIGITAL:
+			rec_status = CEC_OP_RECORD_STATUS_DIG_SERVICE;
 			break;
+		case CEC_OP_RECORD_SRC_ANALOG:
+			rec_status = CEC_OP_RECORD_STATUS_ANA_SERVICE;
+			break;
+		case CEC_OP_RECORD_SRC_EXT_PLUG:
+		case CEC_OP_RECORD_SRC_EXT_PHYS_ADDR:
+			rec_status = CEC_OP_RECORD_STATUS_EXT_INPUT;
+			break;
+		default:
+			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
+			return;
+		}
 		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);
 		return;
 	case CEC_MSG_RECORD_OFF:
-- 
2.25.1

Re: [PATCH v2 1/2] cec: expand One Touch Record TV Screen test

[Hans Verkuil]

On 18/06/2021 01:41, 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 | 53 ++++++++++++++++++++++++++-----
>  utils/cec-follower/cec-tuner.cpp  |  9 ++++--
>  2 files changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 40d8369d..0051d72a 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,52 @@ 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 < 1 || rec_src.type > 5)

Don't use the numbers, use the corresponding defines (CEC_OP_RECORD_SRC_OWN and
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 > 2);

Ditto here...

> +		fail_on_test(rec_src.analog.bcast_system > 8 && rec_src.analog.bcast_system != 0x1f);

...and here.

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;
> 

Re: [PATCH v2 2/2] cec: expand One Touch Record On test

[Hans Verkuil]

Hi Deb,

On 18/06/2021 01:41, Deborah Brouwer wrote:
> Send all Record On source operands and check that the follower responds
> with a corresponding Record Status. Send invalid Record On source
> operands and make sure that the follower returns Feature Abort with
> Invalid Operand.
> 
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp | 107 +++++++++++++++++++++++++++---
>  utils/cec-follower/cec-tuner.cpp  |  28 +++++++-
>  2 files changed, 125 insertions(+), 10 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 0051d72a..2c64d1a0 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -48,6 +48,43 @@ 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, __u8 src, __u8 &rec_status)
> +{
> +	struct cec_msg msg;
> +	struct cec_op_record_src rec_src = {};
> +
> +	cec_msg_init(&msg, me, la);
> +	rec_src.type = src;

You should increase the timeout value to 10s: the spec says that "it may take
several seconds or more before a recorder is able to send an accurate
<Record Status> after receiving a <Record On> message. That 10s is kind of
random since the spec is again very vague about this, but you have to pick
something here.

> +	cec_msg_record_on(&msg, true, &rec_src);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(timed_out_or_abort(&msg));
> +	cec_ops_record_status(&msg, &rec_status);
> +
> +	return OK;
> +}
> +
> +static bool one_touch_rec_common(__u8 rec_status)

This is a poor function name as it does not describe what the function actually does.

> +{
> +	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 true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /* System Information */
>  
>  int system_info_polling(struct node *node, unsigned me, unsigned la, bool interactive)
> @@ -1215,10 +1252,6 @@ 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 = {};
>  
> @@ -1227,15 +1260,68 @@ static int one_touch_rec_on(struct node *node, unsigned me, unsigned la, bool in
>  	cec_msg_record_on(&msg, true, &rec_src);
>  	fail_on_test(!transmit_timeout(node, &msg));
>  	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(cec_has_record(1 << la) || cec_has_backup(1 << la));

You can't test for cec_has_backup here since backup devices do not have to be
recording devices.

>  		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_common(rec_status));

Due to the very vague name 'one_touch_rec_common' I can't really tell what is being
tested here. I think one_touch_rec_common() tests for 'error' states?

Also, since you always test for !one_touch_rec_common() you should consider inverting
the return value from the function.

> +
> +	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_DIGITAL, 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_common(rec_status));
> +
> +	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_ANALOG, 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_common(rec_status));
> +
> +	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_EXT_PLUG, rec_status));
> +	fail_on_test(rec_status != CEC_OP_RECORD_STATUS_EXT_INPUT &&
> +	             rec_status != CEC_OP_RECORD_STATUS_INVALID_EXT_PLUG &&
> +	             !one_touch_rec_common(rec_status));
> +
> +	fail_on_test(one_touch_rec_on_send(node, me, la, CEC_OP_RECORD_SRC_EXT_PHYS_ADDR, 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_common(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));
> +	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));
> +	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);
> +
> +	return OK;
>  }
>  
>  static int one_touch_rec_off(struct node *node, unsigned me, unsigned la, bool interactive)
> @@ -1260,7 +1346,12 @@ 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 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, one_touch_rec_off },
>  };
>  
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index fd11bd10..ebceb064 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -606,10 +606,34 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		return;
>  	}
>  	case CEC_MSG_RECORD_ON:
> -		if (!cec_has_record(1 << me))
> +		if (!cec_has_record(1 << me) && !cec_has_backup(1 << me))

This won't work. cec_has_backup() doesn't mean that the device is actually a
recording device.

Instead, you need to use log_addr_type as returned by CEC_ADAP_G_LOG_ADDRS:

https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/cec/cec-ioc-adap-g-log-addrs.html

So in testProcessing:

__u8 type = laddrs.log_addr_type[0];

Then pass that info to processMsg etc. so it can be tested.

Another things that cec-follower does wrong today is that it has no support
for multiple LAs for a cec device: it just reads laddrs.log_addr[0], and does
not check if other LAs are also available. This is a bug in the follower, this
was simply never implemented. Testing this requires the vivid patch that I
posted on Wednesday.

This should be addressed in a separate patch, but finish the One Touch Record
series first.

> +			break;
> +
> +		struct cec_op_record_src rec_src;
> +		__u8 rec_status;
> +
> +		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;
> +			break;
> +		case CEC_OP_RECORD_SRC_DIGITAL:
> +			rec_status = CEC_OP_RECORD_STATUS_DIG_SERVICE;
>  			break;
> +		case CEC_OP_RECORD_SRC_ANALOG:
> +			rec_status = CEC_OP_RECORD_STATUS_ANA_SERVICE;
> +			break;

For digital and analog recording the service info needs to be checked to see
if it is valid. For analog valid channels emulated by the follower are
defined in analog_freqs_khz. For digital it is defined in digital_arib_data,
digital_atsc_data and digital_dvb_data.

This is probably quite a bit of work for you to figure out how this works, so
I am OK with implementing in a later separate patch.

> +		case CEC_OP_RECORD_SRC_EXT_PLUG:

We should check the plug number. It makes sense if the follower just emulates,
say, 6 external plugs.

> +		case CEC_OP_RECORD_SRC_EXT_PHYS_ADDR:
> +			rec_status = CEC_OP_RECORD_STATUS_EXT_INPUT;

I think we should disallow this for now in the follower. To do this right
it would have to emulate an additional CEC device corresponding to the
physical address. That's just getting to be too complex.

> +			break;
> +		default:
> +			reply_feature_abort(node, &msg, CEC_OP_ABORT_INVALID_OP);
> +			return;
> +		}
>  		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);
>  		return;
>  	case CEC_MSG_RECORD_OFF:
> 

The follower does need to keep track of whether it is already recording and
return CEC_OP_RECORD_STATUS_ALREADY_RECORDING if that's the case. Also
<Standby> should be ignored when recording.

But perhaps this will be implemented in the next patch?

I do think that it makes the patch easier to understand if both Record On and
Record Off are implemented in the same patch: to keep track of the recording
status you need both, so splitting up support for these messages doesn't make
much sense.

Regards,

	Hans