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@xxxxxxxxx>
> ---
> 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
