Hi Deb,
Some more (small) comments:
On 11/06/2021 01:58, Deborah Brouwer wrote:
> Send all Deck Play commands and check that the follower implements them.
> Test that the follower returns Feature Abort for invalid Play operands.
>
> Signed-off-by: Deborah Brouwer <deborahbrouwer3563@xxxxxxxxx>
> ---
> utils/cec-compliance/cec-test.cpp | 100 +++++++++++++++++++++++---
> utils/cec-follower/cec-processing.cpp | 70 +++++++++++++++++-
> 2 files changed, 156 insertions(+), 14 deletions(-)
>
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 83be6b73..f2ec4e90 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -33,6 +33,20 @@ static int deck_status_get(struct node *node, unsigned me, unsigned la, __u8 &de
> return OK;
> }
>
> +static int test_play_mode(struct node *node, unsigned me, unsigned la, __u8 play_mode, __u8 expected)
> +{
> + struct cec_msg msg = {};
> + __u8 deck_status;
> +
> + cec_msg_init(&msg, me, la);
> + cec_msg_play(&msg, play_mode);
> + fail_on_test(!transmit_timeout(node, &msg));
> + fail_on_test(cec_msg_status_is_abort(&msg)); /* Assumes deck has media. */
> + fail_on_test(deck_status_get(node, me, la, deck_status));
> + fail_on_test(deck_status != expected);
> +
> + return OK;
> +}
>
> /* System Information */
>
> @@ -793,24 +807,79 @@ static int deck_ctl_deck_ctl_invalid(struct node *node, unsigned me, unsigned la
> static int deck_ctl_play(struct node *node, unsigned me, unsigned la, bool interactive)
> {
> struct cec_msg msg = {};
> + __u8 deck_status;
>
> cec_msg_init(&msg, me, la);
> - cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_STILL);
> + cec_msg_play(&msg, CEC_OP_PLAY_MODE_PLAY_FWD);
> fail_on_test(!transmit_timeout(node, &msg));
> - if (is_playback_or_rec(la)) {
> - fail_on_test_v2(node->remote[la].cec_version,
> - node->remote[la].has_deck_ctl && unrecognized_op(&msg));
> - fail_on_test_v2(node->remote[la].cec_version,
> - !node->remote[la].has_deck_ctl && !unrecognized_op(&msg));
> - }
> + fail_on_test_v2(node->remote[la].cec_version,
> + node->remote[la].has_deck_ctl && unrecognized_op(&msg));
> + fail_on_test_v2(node->remote[la].cec_version,
> + !node->remote[la].has_deck_ctl && !unrecognized_op(&msg));
> if (unrecognized_op(&msg))
> return OK_NOT_SUPPORTED;
> if (refused(&msg))
> return OK_REFUSED;
> - if (cec_msg_status_is_abort(&msg))
> - return OK_PRESUMED;
> + fail_on_test(deck_status_get(node, me, la, deck_status));
> + if (cec_msg_status_is_abort(&msg)) {
> + if (incorrect_mode(&msg)) {
> + if (deck_status == CEC_OP_DECK_INFO_NO_MEDIA)
> + info("Play Still: no media.\n");
> + else
> + warn("Deck has media but returned Feature Abort with Incorrect Mode.");
> + return OK;
> + }
> + return FAIL;
Do the same reordering as I suggested in the previous patch.
> + }
> + fail_on_test(deck_status != CEC_OP_DECK_INFO_PLAY);
>
> - return OK_PRESUMED;
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_STILL, CEC_OP_DECK_INFO_STILL));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_REV, CEC_OP_DECK_INFO_PLAY_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN, CEC_OP_DECK_INFO_FAST_FWD));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN, CEC_OP_DECK_INFO_FAST_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED, CEC_OP_DECK_INFO_FAST_FWD));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED, CEC_OP_DECK_INFO_FAST_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX, CEC_OP_DECK_INFO_FAST_FWD));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX, CEC_OP_DECK_INFO_FAST_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN, CEC_OP_DECK_INFO_SLOW));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN, CEC_OP_DECK_INFO_SLOW_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED, CEC_OP_DECK_INFO_SLOW));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED, CEC_OP_DECK_INFO_SLOW_REV));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX, CEC_OP_DECK_INFO_SLOW));
> + fail_on_test(test_play_mode(node, me, la, CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX, CEC_OP_DECK_INFO_SLOW_REV));
> +
> + cec_msg_init(&msg, me, la);
> + cec_msg_deck_control(&msg, CEC_OP_DECK_CTL_MODE_STOP);
> + fail_on_test(!transmit_timeout(node, &msg));
> +
> + return OK;
> +}
> +
> +static int deck_ctl_play_invalid(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> + struct cec_msg msg = {};
> +
> + cec_msg_init(&msg, me, la);
> + cec_msg_play(&msg, 4); /* Invalid 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_play(&msg, 8); /* Invalid Operand */
I think it would be more useful to test with 0 instead of 8.
I did consider testing for both, but I think that is a bit overkill.
> + fail_on_test(!transmit_timeout(node, &msg));
> + 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_play(&msg, 0x26); /* Invalid Operand */
> + fail_on_test(!transmit_timeout(node, &msg));
> + fail_on_test(!cec_msg_status_is_abort(&msg));
> + fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
> +
> + return OK;
> }
>
> static const vec_remote_subtests deck_ctl_subtests{
> @@ -825,7 +894,6 @@ static const vec_remote_subtests deck_ctl_subtests{
> deck_ctl_give_status_invalid,
> },
> { "Deck Status", CEC_LOG_ADDR_MASK_ALL, deck_ctl_deck_status },
> - { "Play", CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD, deck_ctl_play },
> {
> "Deck Control",
> CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> @@ -836,6 +904,16 @@ static const vec_remote_subtests deck_ctl_subtests{
> CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> deck_ctl_deck_ctl_invalid,
> },
> + {
> + "Play",
> + CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> + deck_ctl_play,
> + },
> + {
> + "Play Invalid Operand",
> + CEC_LOG_ADDR_MASK_PLAYBACK | CEC_LOG_ADDR_MASK_RECORD,
> + deck_ctl_play_invalid,
> + },
> };
>
> /* Tuner Control */
> diff --git a/utils/cec-follower/cec-processing.cpp b/utils/cec-follower/cec-processing.cpp
> index 4c7ba1d4..e58df85f 100644
> --- a/utils/cec-follower/cec-processing.cpp
> +++ b/utils/cec-follower/cec-processing.cpp
> @@ -553,14 +553,78 @@ static void processMsg(struct node *node, struct cec_msg &msg, unsigned me)
> return;
> }
> case CEC_MSG_PLAY:
> - if (node->has_deck_ctl)
> + if (!node->has_deck_ctl)
> + break;
> +
> + __u8 deck_state;
> + __u8 play_mode;
> +
> + cec_ops_play(&msg, &play_mode);
> +
> + switch (play_mode) {
> + case CEC_OP_PLAY_MODE_PLAY_FWD:
> + /* Play Forward will close tray if open. */
> + deck_state = CEC_OP_DECK_INFO_PLAY;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_REV:
> + if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> + reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> + return;
> + }
> + deck_state = CEC_OP_DECK_INFO_PLAY_REV;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_STILL:
> + /* Play Still will close tray if open. */
> + deck_state = CEC_OP_DECK_INFO_STILL;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MIN:
> + case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MED:
> + case CEC_OP_PLAY_MODE_PLAY_FAST_FWD_MAX:
> + if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> + reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> + return;
> + }
> + deck_state = CEC_OP_DECK_INFO_FAST_FWD;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MIN:
> + case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MED:
> + case CEC_OP_PLAY_MODE_PLAY_FAST_REV_MAX:
> + if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> + reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> + return;
> + }
> + deck_state = CEC_OP_DECK_INFO_FAST_REV;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MIN:
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MED:
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_FWD_MAX:
> + if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> + reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> + return;
> + }
> + deck_state = CEC_OP_DECK_INFO_SLOW;
> + break;
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MIN:
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MED:
> + case CEC_OP_PLAY_MODE_PLAY_SLOW_REV_MAX:
> + if (node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
> + reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
> + return;
> + }
> + deck_state = CEC_OP_DECK_INFO_SLOW_REV;
> + break;
> + default:
> + cec_msg_reply_feature_abort(&msg, CEC_OP_ABORT_INVALID_OP);
> + transmit(node, &msg);
> return;
> - break;
> + }
The NO_MEDIA checks are repeated several times in the switch above. I think that it
is better to do this here:
if (play_mode != CEC_OP_PLAY_MODE_PLAY_FWD &&
play_mode != CEC_OP_PLAY_MODE_PLAY_STILL &&
node->state.deck_state == CEC_OP_DECK_INFO_NO_MEDIA) {
reply_feature_abort(node, &msg, CEC_OP_ABORT_INCORRECT_MODE);
return;
}
> + node->state.deck_skip_start = 0;
> + update_deck_state(node, deck_state);
> + return;
> case CEC_MSG_DECK_CONTROL:
> if (!node->has_deck_ctl)
> break;
>
> - __u8 deck_state;
> __u8 deck_control_mode;
>
> cec_ops_deck_control(&msg, &deck_control_mode);
>
Regards,
Hans
