Some small comments below:
On 17/06/2021 15:21, 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@xxxxxxxxx>
> ---
> utils/cec-compliance/cec-test.cpp | 48 +++++++++++++++++++++++++------
> utils/cec-follower/cec-tuner.cpp | 8 ++++--
> 2 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 40d8369d..9512f319 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,47 @@ 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)
> + 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) {
Add space after 'switch'.
> + 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:
> + fail("Invalid digital service broadcast system operand.\n");
Add break. It is good practice to always add a break, even if it is the last
case of a switch. Someone just might add another case after this in the future,
and suddenly the case above (default:) would fall-through unexpectedly.
> + }
> + }
> +
> + 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..edf5016b 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,11 @@ 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);
> +
> + if (!cec_has_record(1 << la) && !cec_has_backup(1 << la))
> + return;
Add a comment before the if explaining why we check this. Basically copy the
TODO bit that you removed and add it as a comment :-)
Regards,
Hans
> +
> struct cec_op_record_src rec_src = {};
>
> rec_src.type = CEC_OP_RECORD_SRC_OWN;
>
