[PATCH 0/2] cec-compliance: tuner control

[PATCH 0/2] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step increment/decrement as well as performs
cec-compliance tests for: select analog service, give tuner device status, and analog
step increment/decrement.  New tests where added for tuner step increment/decrement and
existing tests where refactored for select analog service and give tuner device status.

---

Jiunn Chang (2):
  cec-follower: add tuner step increment/decrement
  cec-compliance: add/refactor tuner control tests

 utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
 utils/cec-follower/cec-tuner.cpp  |  49 +++++++-
 2 files changed, 186 insertions(+), 44 deletions(-)

-- 
2.23.0

[PATCH 1/2] cec-follower: add tuner step increment/decrement

[Jiunn Chang]

Tuner step increment/decrement will select the next highest or next
lowest service frequency.  There are a total of three possible
frequencies from analog_freqs_khz given a broadcast type and system.

Opcodes implemented:
  - <Tuner Step Increment>
  - <Tuner Step Decrement>

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-tuner.cpp | 49 ++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index acc3fd00..adb2f63c 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	return false;
 }
 
+static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
+{
+	if (info->analog.ana_freq == 0)
+		return -1;
+
+	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
+
+	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
+		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
+
+		if (ana_freq_khz == freq)
+			return i;
+	}
+	return -1;
+}
+
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
 {
 	bool is_bcast = cec_msg_is_broadcast(&msg);
@@ -178,12 +194,39 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 
 	case CEC_MSG_SELECT_DIGITAL_SERVICE:
-	case CEC_MSG_TUNER_STEP_DECREMENT:
-	case CEC_MSG_TUNER_STEP_INCREMENT:
-		if (!cec_has_tuner(1 << me))
+	case CEC_MSG_TUNER_STEP_DECREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
 			break;
+
+		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+		int idx = analog_find_freq_index(info);
+
+		if (idx == -1 || idx == 0)
+			return;
+		if (info->is_analog) {
+			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
+
+			info->analog.ana_freq = (freq * 10) / 625;
+		}
 		return;
+	}
+
+	case CEC_MSG_TUNER_STEP_INCREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
+			break;
 
+		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+		int idx = analog_find_freq_index(info);
+
+		if (idx == -1 || idx == 2)
+			return;
+		if (info->is_analog) {
+			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
+
+			info->analog.ana_freq = (freq * 10) / 625;
+		}
+		return;
+	}
 
 		/*
 		  One Touch Record
-- 
2.23.0

Re: [PATCH 1/2] cec-follower: add tuner step increment/decrement

[Hans Verkuil]

On 9/24/19 4:17 PM, Jiunn Chang wrote:
> Tuner step increment/decrement will select the next highest or next
> lowest service frequency.  There are a total of three possible
> frequencies from analog_freqs_khz given a broadcast type and system.
> 
> Opcodes implemented:
>   - <Tuner Step Increment>
>   - <Tuner Step Decrement>
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-follower/cec-tuner.cpp | 49 ++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index acc3fd00..adb2f63c 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	return false;
>  }
>  
> +static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
> +{
> +	if (info->analog.ana_freq == 0)
> +		return -1;

Can this ever be 0? With the current emulation code this cannot happen, I think.
I'd drop this check.

> +
> +	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
> +
> +	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
> +		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
> +
> +		if (ana_freq_khz == freq)
> +			return i;
> +	}
> +	return -1;

If it can't be found, just return 0. Again, this can't happen with the current code.

> +}
> +
>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
>  {
>  	bool is_bcast = cec_msg_is_broadcast(&msg);
> @@ -178,12 +194,39 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		return;
>  
>  	case CEC_MSG_SELECT_DIGITAL_SERVICE:
> -	case CEC_MSG_TUNER_STEP_DECREMENT:
> -	case CEC_MSG_TUNER_STEP_INCREMENT:
> -		if (!cec_has_tuner(1 << me))
> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>  			break;
> +
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int idx = analog_find_freq_index(info);
> +
> +		if (idx == -1 || idx == 0)

If idx == 0, then just wrap around to the highest index. That's what typically happens
when you press Previous Channel on the TV's remote as well.

> +			return;
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
> +
> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
>  		return;
> +	}
> +
> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
> +			break;
>  
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int idx = analog_find_freq_index(info);
> +
> +		if (idx == -1 || idx == 2)

Same here, just wrap around to 0. And don't use 2, use the NUM_ANALOG_FREQS define.

> +			return;
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
> +
> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
> +		return;
> +	}
>  
>  		/*
>  		  One Touch Record
> 

Regards,

	Hans

[PATCH 2/2] cec-compliance: add/refactor tuner control tests

[Jiunn Chang]

Tests added/refactored for new features added to the cec-follower.

Analog tuner control tests added/refactored:
  - give analog tuner status
  - select tuner analog service
  - analog tuner step decrement
  - analog tuner step increment

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
 1 file changed, 140 insertions(+), 41 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index aece546c..91600d39 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -722,9 +722,19 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
-static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
+			                7668, CEC_OP_BCAST_SYSTEM_PAL_BG); // 479.25 MHz analog frequency
+	fail_on_test(!transmit_timeout(node, &msg));
+	if (unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
 
 	cec_msg_init(&msg, me, la);
 	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
@@ -737,6 +747,14 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
 	if (cec_msg_status_is_abort(&msg))
 		return PRESUMED_OK;
 
+	cec_ops_tuner_device_status(&msg, &info);
+	if (info.analog.ana_bcast_type != CEC_OP_ANA_BCAST_TYPE_CABLE)
+		return FAIL;
+	if (info.analog.ana_freq != 7668)
+		return FAIL;
+	if (info.analog.bcast_system != CEC_OP_BCAST_SYSTEM_PAL_BG)
+		return FAIL;
+
 	return 0;
 }
 
@@ -745,23 +763,24 @@ static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned
 	struct cec_msg msg = {};
 
 	node->remote[la].bcast_sys = ~0;
-	for (unsigned sys = 0; sys <= 8; sys++) {
-		cec_msg_init(&msg, me, la);
-		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
-						7668, sys); // 479.25 MHz analog frequency
-		fail_on_test(!transmit_timeout(node, &msg));
-		if (unrecognized_op(&msg))
-			return NOTSUPPORTED;
-		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
-			info("Tuner supports %s, but cannot select that service.\n",
-			     bcast_system2s(sys));
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 7668, sys); // 479.25 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
+				info("Tuner supports %s, but cannot select that service.\n",
+				bcast_system2s(sys));
+				node->remote[la].bcast_sys = sys;
+				continue;
+			}
+			if (cec_msg_status_is_abort(&msg))
+				continue;
+			info("Tuner supports %s\n", bcast_system2s(sys));
 			node->remote[la].bcast_sys = sys;
-			continue;
 		}
-		if (cec_msg_status_is_abort(&msg))
-			continue;
-		info("Tuner supports %s\n", bcast_system2s(sys));
-		node->remote[la].bcast_sys = sys;
 	}
 
 	if (node->remote[la].bcast_sys == (__u8)~0)
@@ -854,43 +873,123 @@ static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned l
 	return 0;
 }
 
-static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	__u16 freq = 0;
+
+	info.is_analog = true;
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 16000, sys); // 1000 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			freq = info.analog.ana_freq;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_tuner_step_decrement(&msg);
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			if (cec_msg_status_is_abort(&msg))
+				return PRESUMED_OK;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			if (!(info.analog.ana_freq < freq))
+				return FAIL;
+		}
+	}
 
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_decrement(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
+	return 0;
 }
 
-static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	__u16 freq = 0;
+
+	info.is_analog = true;
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 0, sys); // 0 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			freq = info.analog.ana_freq;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_tuner_step_increment(&msg);
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			if (cec_msg_status_is_abort(&msg))
+				return PRESUMED_OK;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			if (!(info.analog.ana_freq > freq))
+				return FAIL;
+		}
+	}
 
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_increment(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
+	return 0;
 }
 
 static struct remote_subtest tuner_ctl_subtests[] = {
-	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
-	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
-	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
+	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_give_status },
+	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_analog_service },
+	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_digital_service },
 	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
-	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
-	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
+	{ "Tuner Analog Step Decrement", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_dec },
+	{ "Tuner Analog Step Increment", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_inc },
 };
 
 
-- 
2.23.0

[PATCH v2 0/2] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step increment/decrement as well as performs
cec-compliance tests for: select analog service, give tuner device status, and analog
step increment/decrement.  New tests where added for tuner step increment/decrement and
existing tests where refactored for select analog service and give tuner device status.

Changes made since v1:
  - Remove redundant error checking
  - Add circular wrap logic to tuner step increment/decrement

---

Jiunn Chang (2):
  cec-follower: add tuner step increment/decrement
  cec-compliance: add/refactor tuner control tests

 utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
 utils/cec-follower/cec-tuner.cpp  |  47 +++++++-
 2 files changed, 184 insertions(+), 44 deletions(-)

-- 
2.23.0

[PATCH v3 0/3] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step increment/decrement as well as
performs cec-compliance tests.  Tests where refactored into a single function,
tuner_ctl_test(), to test all tuner features.

---

Changes made since v2:
  - Fix bugs for tuner emulation
  - Add freq_idx to cec-follower state
  - Refactor tuner tests into a single tuner_ctl_test()

Changes made since v1:
  - Remove redundant error checking
  - Add circular wrap logic to tuner step increment/decrement

Jiunn Chang (3):
  cec-follower: fix bugs for tuner emulation
  cec-follower: add tuner step increment/decrement
  cec-compliance: refactor tuner control tests

 utils/cec-compliance/cec-test.cpp   | 184 ++++++++--------------------
 utils/cec-follower/cec-follower.cpp |   2 +-
 utils/cec-follower/cec-follower.h   |   3 +-
 utils/cec-follower/cec-tuner.cpp    |  93 +++++++++++---
 4 files changed, 135 insertions(+), 147 deletions(-)

-- 
2.23.0

[PATCH v4 0/3] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step features and cec-compliance tests.
Added tuner_ctl_test() to test new tuner features implemented in the cec-follower.

---

changes made since v3:
  - Correct commit message for tuner emulation bug fix
  - Refactor analog_get_nearest_freq_idx() to return freq index
  - Refactor analog_get_type_system_idx() to set device info
  - Add macro ARRAY_SIZE_ANA_FREQS
  - Refactor tuner_ctl_test() to use std::vector

Changes made since v2:
  - Fix bugs for tuner emulation
  - Add freq_idx to cec-follower state
  - Refactor tuner tests into a single tuner_ctl_test()

Changes made since v1:
  - Remove redundant error checking
  - Add circular wrap logic to tuner step increment/decrement

Jiunn Chang (3):
  cec-follower: fix bugs for tuner emulation
  cec-follower: add tuner step increment/decrement
  cec-compliance: add tuner control test

 utils/cec-compliance/cec-test.cpp   | 91 +++++++++++++++++++++++++++++
 utils/cec-follower/cec-follower.cpp |  2 +-
 utils/cec-follower/cec-follower.h   |  3 +-
 utils/cec-follower/cec-tuner.cpp    | 73 +++++++++++++++++------
 4 files changed, 150 insertions(+), 19 deletions(-)

-- 
2.23.0

[PATCH v5 0/2] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step features and cec-compliance tests.
Added tuner_ctl_test() to test new tuner features implemented in the cec-follower.

---

Changes made since v4:
  - Add TOT_ANALOG_FREQS macro: hverkuil
  - Rename function to analog_update_tuner_dev_info(): hverkuil
  - Set freq_idx in analog_update_tuner_dev_info(): hverkuil
  - Reuse info variable in tuner_ctl_test(): hverkuil
  - Refactor tuner_ctl_test() to leverage timed_out_or_abort() and
    abort_reason(): hverkuil

changes made since v3:
  - Correct commit message for tuner emulation bug fix
  - Refactor analog_get_nearest_freq_idx() to return freq index
  - Refactor analog_get_type_system_idx() to set device info
  - Add macro ARRAY_SIZE_ANA_FREQS
  - Refactor tuner_ctl_test() to use std::vector

Changes made since v2:
  - Fix bugs for tuner emulation
  - Add freq_idx to cec-follower state
  - Refactor tuner tests into a single tuner_ctl_test()

Changes made since v1:
  - Remove redundant error checking
  - Add circular wrap logic to tuner step increment/decrement

Jiunn Chang (2):
  cec-follower: add tuner step increment/decrement
  Add test for new features in cec-follower

 utils/cec-compliance/cec-test.cpp   | 74 +++++++++++++++++++++++++++++
 utils/cec-follower/cec-follower.cpp |  2 +-
 utils/cec-follower/cec-follower.h   |  3 +-
 utils/cec-follower/cec-tuner.cpp    | 67 ++++++++++++++++++++------
 4 files changed, 129 insertions(+), 17 deletions(-)

-- 
2.23.0

[PATCH v6 0/1] cec-compliance: tuner control

[Jiunn Chang]

This patch series implements analog tuner step features and cec-compliance tests.
Added tuner_ctl_test() to test new tuner features implemented in the cec-follower.

---

Changes made since v5:
  - Add missing fail_on_test(cec_msg_status_is_abort(&msg)): hverkuil
  - Remove redundant test unrecognized_op(&msg): hverkuil
  - Refactor tuner_ctl_test to use memcmp(): hverkuil

Changes made since v4:
  - Add TOT_ANALOG_FREQS macro: hverkuil
  - Rename function to analog_update_tuner_dev_info(): hverkuil
  - Set freq_idx in analog_update_tuner_dev_info(): hverkuil
  - Reuse info variable in tuner_ctl_test(): hverkuil
  - Refactor tuner_ctl_test() to leverage timed_out_or_abort() and
    abort_reason(): hverkuil

changes made since v3:
  - Correct commit message for tuner emulation bug fix
  - Refactor analog_get_nearest_freq_idx() to return freq index
  - Refactor analog_get_type_system_idx() to set device info
  - Add macro ARRAY_SIZE_ANA_FREQS
  - Refactor tuner_ctl_test() to use std::vector

Changes made since v2:
  - Fix bugs for tuner emulation
  - Add freq_idx to cec-follower state
  - Refactor tuner tests into a single tuner_ctl_test()

Changes made since v1:
  - Remove redundant error checking
  - Add circular wrap logic to tuner step increment/decrement

Jiunn Chang (1):
  Add test for new features in cec-follower

 utils/cec-compliance/cec-test.cpp | 75 +++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

-- 
2.23.0

[PATCH v6 1/1] Add test for new features in cec-follower

[Jiunn Chang]

Analog tuner control test tuner_ctl_test():
  - give analog tuner status
  - select tuner analog service
  - analog tuner step features

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 75 +++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index ec07d886..1fd0ea92 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -16,6 +16,7 @@
 #include <sys/ioctl.h>
 #include <config.h>
 #include <sstream>
+#include <vector>
 
 #include "cec-compliance.h"
 
@@ -722,6 +723,79 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
+static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	std::vector<struct cec_op_tuner_device_info> info_vec;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(cec_msg_status_is_abort(&msg));
+	if (timed_out(&msg) || unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
+	cec_ops_tuner_device_status(&msg, &info);
+	info_vec.push_back(info);
+
+	while (1) {
+		cec_msg_init(&msg, me, la);
+		cec_msg_tuner_step_increment(&msg);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(cec_msg_status_is_abort(&msg));
+		if (cec_msg_status_is_abort(&msg)) {
+			fail_on_test(abort_reason(&msg) == CEC_OP_ABORT_UNRECOGNIZED_OP);
+			if (abort_reason(&msg) == CEC_OP_ABORT_REFUSED) {
+				warn("Tuner step increment does not wrap.\n");
+				break;
+			} else {
+				warn("Tuner at end of service list did not receive feature abort refused.\n");
+				break;
+			}
+		}
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out_or_abort(&msg));
+		info = {};
+		cec_ops_tuner_device_status(&msg, &info);
+		if (!memcmp(&info, &info_vec[0], sizeof(info)))
+			break;
+		info_vec.push_back(info);
+	}
+
+	for (std::vector<struct cec_op_tuner_device_info>::iterator iter = info_vec.begin();
+			iter != info_vec.end(); iter++) {
+		cec_msg_init(&msg, me, la);
+		cec_msg_select_analogue_service(&msg, iter->analog.ana_bcast_type,
+			iter->analog.ana_freq, iter->analog.bcast_system);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(cec_msg_status_is_abort(&msg));
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out_or_abort(&msg));
+		info = {};
+		cec_ops_tuner_device_status(&msg, &info);
+		fail_on_test(memcmp(&info, &(*iter), sizeof(info)));
+		if (iter->is_analog) {
+			double freq_mhz = (iter->analog.ana_freq * 625) / 10000.0;
+
+			info("Analog channel freq: %.2f MHz\n", freq_mhz);
+		}
+	}
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
+	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 0;
+}
+
 static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
@@ -885,6 +959,7 @@ static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool
 }
 
 static struct remote_subtest tuner_ctl_subtests[] = {
+	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
 	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
 	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
 	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
-- 
2.23.0

[PATCH v5 1/2] cec-follower: add tuner step increment/decrement

[Jiunn Chang]

Tuner step feature will select the next highest or lowest service
frequency.  There are a total of three possible frequencies given
a broadcast type and system for a total of 81 analog channels.

Opcodes implemented:
  - <Tuner Step Increment>
  - <Tuner Step Decrement>

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-follower.cpp |  2 +-
 utils/cec-follower/cec-follower.h   |  3 +-
 utils/cec-follower/cec-tuner.cpp    | 67 ++++++++++++++++++++++-------
 3 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 789fef4a..b208d14e 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -298,7 +298,7 @@ void state_init(struct node &node)
 	node.state.sac_active = false;
 	node.state.volume = 50;
 	node.state.mute = false;
-	analog_tuner_init(&node.state.tuner_dev_info);
+	analog_tuner_init(&node.state);
 }
 
 int main(int argc, char **argv)
diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
index 0bd45452..04019c9a 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -53,6 +53,7 @@ struct state {
 	unsigned rc_press_hold_count;
 	unsigned rc_duration_sum;
 	struct cec_op_tuner_device_info tuner_dev_info;
+	unsigned int freq_idx;
 };
 
 struct node {
@@ -222,7 +223,7 @@ std::string opcode2s(const struct cec_msg *msg);
 void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
 
 // cec-tuner.cpp
-void analog_tuner_init(struct cec_op_tuner_device_info *tuner_dev_info);
+void analog_tuner_init(struct state *state);
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
 
 // CEC processing
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index d74981b2..46acccf4 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -8,7 +8,8 @@
 
 #include "cec-follower.h"
 
-#define NUM_ANALOG_FREQS	3
+#define NUM_ANALOG_FREQS 3
+#define TOT_ANALOG_FREQS (sizeof(analog_freqs_khz) / sizeof(analog_freqs_khz[0][0][0]))
 
 /*
  * This table contains analog television channel frequencies in KHz.  There are
@@ -90,10 +91,12 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
 	}
 };
 
-void analog_tuner_init(struct cec_op_tuner_device_info *info)
+void analog_tuner_init(struct state *state)
 {
+	struct cec_op_tuner_device_info *info = &state->tuner_dev_info;
 	unsigned int freq_khz;
 
+	state->freq_idx = 0;
 	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
 	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
 	info->is_analog = true;
@@ -103,35 +106,52 @@ void analog_tuner_init(struct cec_op_tuner_device_info *info)
 	info->analog.ana_freq = (freq_khz * 10) / 625;
 }
 
-static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
-                                            int ana_freq_khz)
+static unsigned int analog_get_nearest_freq_idx(__u8 ana_bcast_type, __u8 ana_bcast_system,
+						int ana_freq_khz)
 {
 	int nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
+	unsigned int offset = 0;
 
 	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
 		int freq = analog_freqs_khz[ana_bcast_type][ana_bcast_system][i];
 
-		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest))
+		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest)) {
 			nearest = freq;
+			offset = i;
+		}
 	}
-	return nearest;
+	return NUM_ANALOG_FREQS * ((ana_bcast_type * 9) + ana_bcast_system) + offset;
 }
 
-static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
+static void analog_update_tuner_dev_info(struct node *node, unsigned int idx)
 {
 	struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+	unsigned int tot_freqs = NUM_ANALOG_FREQS * 9;
+	unsigned int offset;
+	unsigned int freq_khz;
+
+	node->state.freq_idx = idx;
+	info->analog.ana_bcast_type = node->state.freq_idx / tot_freqs;
+	info->analog.bcast_system =
+		(node->state.freq_idx - (tot_freqs * info->analog.ana_bcast_type)) / NUM_ANALOG_FREQS;
+	offset = node->state.freq_idx % NUM_ANALOG_FREQS;
+	freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][offset];
+	info->analog.ana_freq = (freq_khz * 10) / 625;
+}
+
+static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
+{
 	__u8 type;
 	__u16 freq;
 	__u8 system;
+	unsigned int idx;
 
 	cec_ops_select_analogue_service(msg, &type, &freq, &system);
 	if (type < 3 && system < 9) {
 		int freq_khz = (freq * 625) / 10;
-		unsigned int nearest = analog_get_nearest_freq(type, system,
-							       freq_khz);
-		info->analog.ana_bcast_type = type;
-		info->analog.ana_freq = (nearest * 10) / 625;
-		info->analog.bcast_system = system;
+
+		idx = analog_get_nearest_freq_idx(type, system, freq_khz);
+		analog_update_tuner_dev_info(node, idx);
 		return true;
 	}
 	return false;
@@ -180,12 +200,29 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 
 	case CEC_MSG_SELECT_DIGITAL_SERVICE:
-	case CEC_MSG_TUNER_STEP_DECREMENT:
-	case CEC_MSG_TUNER_STEP_INCREMENT:
-		if (!cec_has_tuner(1 << me))
+	case CEC_MSG_TUNER_STEP_DECREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
 			break;
+
+		if (node->state.freq_idx == 0)
+			node->state.freq_idx = TOT_ANALOG_FREQS - 1;
+		else
+			node->state.freq_idx--;
+		analog_update_tuner_dev_info(node, node->state.freq_idx);
 		return;
+	}
 
+	case CEC_MSG_TUNER_STEP_INCREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
+			break;
+
+		if (node->state.freq_idx == TOT_ANALOG_FREQS - 1)
+			node->state.freq_idx = 0;
+		else
+			node->state.freq_idx++;
+		analog_update_tuner_dev_info(node, node->state.freq_idx);
+		return;
+	}
 
 		/*
 		  One Touch Record
-- 
2.23.0

[PATCH v5 2/2] Add test for new features in cec-follower

[Jiunn Chang]

Analog tuner control test tuner_ctl_test():
  - give analog tuner status
  - select tuner analog service
  - analog tuner step features

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 74 +++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index 017d1ef8..669df4b7 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -16,6 +16,7 @@
 #include <sys/ioctl.h>
 #include <config.h>
 #include <sstream>
+#include <vector>
 
 #include "cec-compliance.h"
 
@@ -722,6 +723,78 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
+static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	__u16 start_freq;
+	std::vector<struct cec_op_tuner_device_info> info_vec;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+	fail_on_test(!transmit_timeout(node, &msg));
+	if (timed_out(&msg) || unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
+	cec_ops_tuner_device_status(&msg, &info);
+	start_freq = info.analog.ana_freq;
+	info_vec.push_back(info);
+
+	while (1) {
+		cec_msg_init(&msg, me, la);
+		cec_msg_tuner_step_increment(&msg);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(cec_msg_status_is_abort(&msg));
+		if (cec_msg_status_is_abort(&msg)) {
+			fail_on_test(abort_reason(&msg) == CEC_OP_ABORT_UNRECOGNIZED_OP);
+			if (abort_reason(&msg) == CEC_OP_ABORT_REFUSED) {
+				warn("Tuner step increment does not wrap.\n");
+				break;
+			} else {
+				warn("Tuner at end of service list did not receive feature abort refused.\n");
+				break;
+			}
+		}
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out_or_abort(&msg));
+		cec_ops_tuner_device_status(&msg, &info);
+		if (info.analog.ana_freq == start_freq)
+			break;
+		info_vec.push_back(info);
+	}
+
+	for (std::vector<struct cec_op_tuner_device_info>::iterator iter = info_vec.begin();
+			iter != info_vec.end(); iter++) {
+		cec_msg_init(&msg, me, la);
+		cec_msg_select_analogue_service(&msg, iter->analog.ana_bcast_type,
+			iter->analog.ana_freq, iter->analog.bcast_system);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(cec_msg_status_is_abort(&msg));
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out_or_abort(&msg));
+		cec_ops_tuner_device_status(&msg, &info);
+		fail_on_test(info.analog.ana_freq != iter->analog.ana_freq);
+		if (iter->is_analog) {
+			double freq_mhz = (iter->analog.ana_freq * 625) / 10000.0;
+			info("Analog channel freq: %.2f MHz\n", freq_mhz);
+		}
+	}
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+	fail_on_test(unrecognized_op(&msg));
+	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
+
+	return 0;
+}
+
 static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
@@ -885,6 +958,7 @@ static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool
 }
 
 static struct remote_subtest tuner_ctl_subtests[] = {
+	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
 	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
 	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
 	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
-- 
2.23.0

Re: [PATCH v5 2/2] Add test for new features in cec-follower

[Hans Verkuil]

On 10/3/19 5:18 AM, Jiunn Chang wrote:
> Analog tuner control test tuner_ctl_test():
>   - give analog tuner status
>   - select tuner analog service
>   - analog tuner step features
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp | 74 +++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index 017d1ef8..669df4b7 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -16,6 +16,7 @@
>  #include <sys/ioctl.h>
>  #include <config.h>
>  #include <sstream>
> +#include <vector>
>  
>  #include "cec-compliance.h"
>  
> @@ -722,6 +723,78 @@ static struct remote_subtest deck_ctl_subtests[] = {
>    TODO: These are very rudimentary tests which should be expanded.
>   */
>  
> +static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> +	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	__u16 start_freq;
> +	std::vector<struct cec_op_tuner_device_info> info_vec;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	if (timed_out(&msg) || unrecognized_op(&msg))
> +		return NOTSUPPORTED;
> +	if (refused(&msg))
> +		return REFUSED;

You're missing a 'fail_on_test(cec_msg_status_is_abort(&msg));' line here,
since any other abort reason means the test should fail.

> +	cec_ops_tuner_device_status(&msg, &info);
> +	start_freq = info.analog.ana_freq;
> +	info_vec.push_back(info);
> +
> +	while (1) {
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_tuner_step_increment(&msg);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(cec_msg_status_is_abort(&msg));
> +		if (cec_msg_status_is_abort(&msg)) {
> +			fail_on_test(abort_reason(&msg) == CEC_OP_ABORT_UNRECOGNIZED_OP);
> +			if (abort_reason(&msg) == CEC_OP_ABORT_REFUSED) {
> +				warn("Tuner step increment does not wrap.\n");
> +				break;
> +			} else {
> +				warn("Tuner at end of service list did not receive feature abort refused.\n");
> +				break;
> +			}
> +		}
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out_or_abort(&msg));
> +		cec_ops_tuner_device_status(&msg, &info);
> +		if (info.analog.ana_freq == start_freq)
> +			break;
> +		info_vec.push_back(info);
> +	}
> +
> +	for (std::vector<struct cec_op_tuner_device_info>::iterator iter = info_vec.begin();
> +			iter != info_vec.end(); iter++) {
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_select_analogue_service(&msg, iter->analog.ana_bcast_type,
> +			iter->analog.ana_freq, iter->analog.bcast_system);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(cec_msg_status_is_abort(&msg));
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out_or_abort(&msg));
> +		cec_ops_tuner_device_status(&msg, &info);
> +		fail_on_test(info.analog.ana_freq != iter->analog.ana_freq);
> +		if (iter->is_analog) {
> +			double freq_mhz = (iter->analog.ana_freq * 625) / 10000.0;

Add newline.

> +			info("Analog channel freq: %.2f MHz\n", freq_mhz);
> +		}
> +	}
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(!cec_msg_status_is_abort(&msg));
> +	fail_on_test(unrecognized_op(&msg));

This test is unnecessary, since the next test will catch this.

> +	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
> +
> +	return 0;
> +}
> +
>  static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> @@ -885,6 +958,7 @@ static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool
>  }
>  
>  static struct remote_subtest tuner_ctl_subtests[] = {
> +	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
>  	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
>  	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
>  	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
> 

Regards,

	Hans

[PATCH v4 1/3] cec-follower: fix bugs for tuner emulation

[Jiunn Chang]

Fix bugs for tuner emulation:
  - analog_tuner_init() needs to store ana_freq as multiples of 62.5 kHz
  - analog_set_tuner_dev_info() needs to check the broadcast type and
    broadcast system are valid before getting the nearest frequency

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-tuner.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index acc3fd00..d74981b2 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -92,13 +92,15 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
 
 void analog_tuner_init(struct cec_op_tuner_device_info *info)
 {
+	unsigned int freq_khz;
+
 	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
 	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
 	info->is_analog = true;
 	info->analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
 	info->analog.bcast_system = CEC_OP_BCAST_SYSTEM_PAL_BG;
-	info->analog.ana_freq =
-		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
+	freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
+	info->analog.ana_freq = (freq_khz * 10) / 625;
 }
 
 static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
@@ -123,7 +125,7 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	__u8 system;
 
 	cec_ops_select_analogue_service(msg, &type, &freq, &system);
-	if (info->analog.ana_bcast_type < 3 && info->analog.bcast_system < 9) {
+	if (type < 3 && system < 9) {
 		int freq_khz = (freq * 625) / 10;
 		unsigned int nearest = analog_get_nearest_freq(type, system,
 							       freq_khz);
-- 
2.23.0

[PATCH v4 2/3] cec-follower: add tuner step increment/decrement

[Jiunn Chang]

Tuner step increment/decrement will select the next highest or next
lowest service frequency.  There are a total of three possible
frequencies given a broadcast type and system for a total of 81 analog
channels.

Opcodes implemented:
  - <Tuner Step Increment>
  - <Tuner Step Decrement>

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-follower.cpp |  2 +-
 utils/cec-follower/cec-follower.h   |  3 +-
 utils/cec-follower/cec-tuner.cpp    | 65 ++++++++++++++++++++++-------
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 4243fdd9..8375404e 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -298,7 +298,7 @@ void state_init(struct node &node)
 	node.state.sac_active = false;
 	node.state.volume = 50;
 	node.state.mute = false;
-	analog_tuner_init(&node.state.tuner_dev_info);
+	analog_tuner_init(&node.state);
 }
 
 int main(int argc, char **argv)
diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
index a53c16fe..d8ac4e77 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -52,6 +52,7 @@ struct state {
 	unsigned rc_press_hold_count;
 	unsigned rc_duration_sum;
 	struct cec_op_tuner_device_info tuner_dev_info;
+	unsigned int freq_idx;
 };
 
 struct node {
@@ -221,7 +222,7 @@ std::string opcode2s(const struct cec_msg *msg);
 void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
 
 // cec-tuner.cpp
-void analog_tuner_init(struct cec_op_tuner_device_info *tuner_dev_info);
+void analog_tuner_init(struct state *state);
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
 
 // CEC processing
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index d74981b2..6f14b059 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -9,6 +9,8 @@
 #include "cec-follower.h"
 
 #define NUM_ANALOG_FREQS	3
+#define ARRAY_SIZE_ANA_FREQS(x) \
+	ARRAY_SIZE(x) * ARRAY_SIZE(*x) * ARRAY_SIZE(**x)
 
 /*
  * This table contains analog television channel frequencies in KHz.  There are
@@ -90,10 +92,12 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
 	}
 };
 
-void analog_tuner_init(struct cec_op_tuner_device_info *info)
+void analog_tuner_init(struct state *state)
 {
+	struct cec_op_tuner_device_info *info = &state->tuner_dev_info;
 	unsigned int freq_khz;
 
+	state->freq_idx = 0;
 	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
 	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
 	info->is_analog = true;
@@ -103,23 +107,41 @@ void analog_tuner_init(struct cec_op_tuner_device_info *info)
 	info->analog.ana_freq = (freq_khz * 10) / 625;
 }
 
-static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
-                                            int ana_freq_khz)
+static unsigned int analog_get_nearest_freq_idx(__u8 ana_bcast_type, __u8 ana_bcast_system,
+						int ana_freq_khz)
 {
 	int nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
+	unsigned int offset = 0;
 
 	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
 		int freq = analog_freqs_khz[ana_bcast_type][ana_bcast_system][i];
 
-		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest))
+		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest)) {
 			nearest = freq;
+			offset++;
+		}
 	}
-	return nearest;
+	return NUM_ANALOG_FREQS * ((ana_bcast_type * 9) + ana_bcast_system) + offset;
 }
 
-static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
+static void analog_get_type_system_idx(struct node *node)
 {
 	struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+	unsigned int tot_freqs = NUM_ANALOG_FREQS * 9;
+	unsigned int freq_idx = node->state.freq_idx;
+	unsigned int freq_khz;
+	unsigned int idx;
+
+	info->analog.ana_bcast_type = freq_idx / tot_freqs;
+	info->analog.bcast_system =
+		(freq_idx - (tot_freqs * info->analog.ana_bcast_type)) / NUM_ANALOG_FREQS;
+	idx = freq_idx % NUM_ANALOG_FREQS;
+	freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][idx];
+	info->analog.ana_freq = (freq_khz * 10) / 625;
+}
+
+static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
+{
 	__u8 type;
 	__u16 freq;
 	__u8 system;
@@ -127,11 +149,9 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	cec_ops_select_analogue_service(msg, &type, &freq, &system);
 	if (type < 3 && system < 9) {
 		int freq_khz = (freq * 625) / 10;
-		unsigned int nearest = analog_get_nearest_freq(type, system,
-							       freq_khz);
-		info->analog.ana_bcast_type = type;
-		info->analog.ana_freq = (nearest * 10) / 625;
-		info->analog.bcast_system = system;
+
+		node->state.freq_idx = analog_get_nearest_freq_idx(type, system, freq_khz);
+		analog_get_type_system_idx(node);
 		return true;
 	}
 	return false;
@@ -180,12 +200,29 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 
 	case CEC_MSG_SELECT_DIGITAL_SERVICE:
-	case CEC_MSG_TUNER_STEP_DECREMENT:
-	case CEC_MSG_TUNER_STEP_INCREMENT:
-		if (!cec_has_tuner(1 << me))
+	case CEC_MSG_TUNER_STEP_DECREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
 			break;
+
+		if (node->state.freq_idx == 0)
+			node->state.freq_idx = ARRAY_SIZE_ANA_FREQS(analog_freqs_khz) - 1;
+		else
+			node->state.freq_idx--;
+		analog_get_type_system_idx(node);
 		return;
+	}
 
+	case CEC_MSG_TUNER_STEP_INCREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
+			break;
+
+		if (node->state.freq_idx == ARRAY_SIZE_ANA_FREQS(analog_freqs_khz) - 1)
+			node->state.freq_idx = 0;
+		else
+			node->state.freq_idx++;
+		analog_get_type_system_idx(node);
+		return;
+	}
 
 		/*
 		  One Touch Record
-- 
2.23.0

Re: [PATCH v4 2/3] cec-follower: add tuner step increment/decrement

[Hans Verkuil]

On 10/1/19 5:18 AM, Jiunn Chang wrote:
> Tuner step increment/decrement will select the next highest or next
> lowest service frequency.  There are a total of three possible
> frequencies given a broadcast type and system for a total of 81 analog
> channels.
> 
> Opcodes implemented:
>   - <Tuner Step Increment>
>   - <Tuner Step Decrement>
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-follower/cec-follower.cpp |  2 +-
>  utils/cec-follower/cec-follower.h   |  3 +-
>  utils/cec-follower/cec-tuner.cpp    | 65 ++++++++++++++++++++++-------
>  3 files changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
> index 4243fdd9..8375404e 100644
> --- a/utils/cec-follower/cec-follower.cpp
> +++ b/utils/cec-follower/cec-follower.cpp
> @@ -298,7 +298,7 @@ void state_init(struct node &node)
>  	node.state.sac_active = false;
>  	node.state.volume = 50;
>  	node.state.mute = false;
> -	analog_tuner_init(&node.state.tuner_dev_info);
> +	analog_tuner_init(&node.state);
>  }
>  
>  int main(int argc, char **argv)
> diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
> index a53c16fe..d8ac4e77 100644
> --- a/utils/cec-follower/cec-follower.h
> +++ b/utils/cec-follower/cec-follower.h
> @@ -52,6 +52,7 @@ struct state {
>  	unsigned rc_press_hold_count;
>  	unsigned rc_duration_sum;
>  	struct cec_op_tuner_device_info tuner_dev_info;
> +	unsigned int freq_idx;
>  };
>  
>  struct node {
> @@ -221,7 +222,7 @@ std::string opcode2s(const struct cec_msg *msg);
>  void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
>  
>  // cec-tuner.cpp
> -void analog_tuner_init(struct cec_op_tuner_device_info *tuner_dev_info);
> +void analog_tuner_init(struct state *state);
>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
>  
>  // CEC processing
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index d74981b2..6f14b059 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -9,6 +9,8 @@
>  #include "cec-follower.h"
>  
>  #define NUM_ANALOG_FREQS	3
> +#define ARRAY_SIZE_ANA_FREQS(x) \
> +	ARRAY_SIZE(x) * ARRAY_SIZE(*x) * ARRAY_SIZE(**x)

This can be a bit easier:

#define TOT_ANALOG_FREQS (sizeof(analog_freqs_khz) / sizeof(analog_freqs_khz[0][0][0]))

>  
>  /*
>   * This table contains analog television channel frequencies in KHz.  There are
> @@ -90,10 +92,12 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
>  	}
>  };
>  
> -void analog_tuner_init(struct cec_op_tuner_device_info *info)
> +void analog_tuner_init(struct state *state)
>  {
> +	struct cec_op_tuner_device_info *info = &state->tuner_dev_info;
>  	unsigned int freq_khz;
>  
> +	state->freq_idx = 0;
>  	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
>  	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
>  	info->is_analog = true;
> @@ -103,23 +107,41 @@ void analog_tuner_init(struct cec_op_tuner_device_info *info)
>  	info->analog.ana_freq = (freq_khz * 10) / 625;
>  }
>  
> -static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
> -                                            int ana_freq_khz)
> +static unsigned int analog_get_nearest_freq_idx(__u8 ana_bcast_type, __u8 ana_bcast_system,
> +						int ana_freq_khz)
>  {
>  	int nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
> +	unsigned int offset = 0;
>  
>  	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
>  		int freq = analog_freqs_khz[ana_bcast_type][ana_bcast_system][i];
>  
> -		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest))
> +		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest)) {
>  			nearest = freq;
> +			offset++;

This should be 'offset = i;'

> +		}
>  	}
> -	return nearest;
> +	return NUM_ANALOG_FREQS * ((ana_bcast_type * 9) + ana_bcast_system) + offset;
>  }
>  
> -static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
> +static void analog_get_type_system_idx(struct node *node)

'get' implies that data is returned, and that's not the case here.

How about analog_update_tuner_dev_info(struct node *node, unsigned int idx);

So the index is passed as an argument and this function sets state.freq_idx as well.

>  {
>  	struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +	unsigned int tot_freqs = NUM_ANALOG_FREQS * 9;
> +	unsigned int freq_idx = node->state.freq_idx;
> +	unsigned int freq_khz;
> +	unsigned int idx;
> +
> +	info->analog.ana_bcast_type = freq_idx / tot_freqs;
> +	info->analog.bcast_system =
> +		(freq_idx - (tot_freqs * info->analog.ana_bcast_type)) / NUM_ANALOG_FREQS;
> +	idx = freq_idx % NUM_ANALOG_FREQS;
> +	freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][idx];
> +	info->analog.ana_freq = (freq_khz * 10) / 625;
> +}
> +
> +static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
> +{
>  	__u8 type;
>  	__u16 freq;
>  	__u8 system;
> @@ -127,11 +149,9 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	cec_ops_select_analogue_service(msg, &type, &freq, &system);
>  	if (type < 3 && system < 9) {
>  		int freq_khz = (freq * 625) / 10;
> -		unsigned int nearest = analog_get_nearest_freq(type, system,
> -							       freq_khz);
> -		info->analog.ana_bcast_type = type;
> -		info->analog.ana_freq = (nearest * 10) / 625;
> -		info->analog.bcast_system = system;
> +
> +		node->state.freq_idx = analog_get_nearest_freq_idx(type, system, freq_khz);
> +		analog_get_type_system_idx(node);
>  		return true;
>  	}
>  	return false;
> @@ -180,12 +200,29 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		return;
>  
>  	case CEC_MSG_SELECT_DIGITAL_SERVICE:
> -	case CEC_MSG_TUNER_STEP_DECREMENT:
> -	case CEC_MSG_TUNER_STEP_INCREMENT:
> -		if (!cec_has_tuner(1 << me))
> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>  			break;
> +
> +		if (node->state.freq_idx == 0)
> +			node->state.freq_idx = ARRAY_SIZE_ANA_FREQS(analog_freqs_khz) - 1;
> +		else
> +			node->state.freq_idx--;
> +		analog_get_type_system_idx(node);
>  		return;
> +	}
>  
> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
> +			break;
> +
> +		if (node->state.freq_idx == ARRAY_SIZE_ANA_FREQS(analog_freqs_khz) - 1)
> +			node->state.freq_idx = 0;
> +		else
> +			node->state.freq_idx++;
> +		analog_get_type_system_idx(node);
> +		return;
> +	}
>  
>  		/*
>  		  One Touch Record
> 

It's getting close...

Regards,

	Hans

[PATCH v4 3/3] cec-compliance: add tuner control test

[Jiunn Chang]

Add test for new features added to cec-follower.

Analog tuner control test tuner_ctl_test():
  - give analog tuner status
  - select tuner analog service
  - analog tuner step features

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 91 +++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index aece546c..17f72cc6 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -16,6 +16,7 @@
 #include <sys/ioctl.h>
 #include <config.h>
 #include <sstream>
+#include <vector>
 
 #include "cec-compliance.h"
 
@@ -722,6 +723,95 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
+static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
+{
+	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	std::vector<struct cec_op_tuner_device_info> info_vec;
+	__u16 start_freq;
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+	fail_on_test(!transmit_timeout(node, &msg));
+	fail_on_test(timed_out(&msg));
+	if (unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
+	cec_ops_tuner_device_status(&msg, &info);
+	start_freq = info.analog.ana_freq;
+	info_vec.push_back(info);
+
+	while (1) {
+		struct cec_op_tuner_device_info new_info;
+
+		cec_msg_init(&msg, me, la);
+		cec_msg_tuner_step_increment(&msg);
+		fail_on_test(!transmit_timeout(node, &msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out(&msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_ops_tuner_device_status(&msg, &new_info);
+		if (new_info.analog.ana_freq == start_freq)
+			break;
+		if (new_info.analog.ana_freq == info_vec.back().analog.ana_freq) {
+			warn("Tuner channel step increment does not wrap.\n");
+			break;
+		}
+		info_vec.push_back(new_info);
+	}
+
+	
+	for (std::vector<struct cec_op_tuner_device_info>::iterator iter = info_vec.begin();
+			iter != info_vec.end(); iter++) {
+		struct cec_op_tuner_device_info current;
+
+		cec_msg_init(&msg, me, la);
+		cec_msg_select_analogue_service(&msg, iter->analog.ana_bcast_type,
+			iter->analog.ana_freq, iter->analog.bcast_system);
+		fail_on_test(!transmit_timeout(node, &msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out(&msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_ops_tuner_device_status(&msg, &current);
+		fail_on_test(current.analog.ana_freq != iter->analog.ana_freq);
+		if (iter->is_analog) {
+			float freq_mhz = (iter->analog.ana_freq * 625) / 10000;
+			info("Analog channel freq: %.2f MHz\n", freq_mhz);
+		}
+	}
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
+	fail_on_test(!transmit_timeout(node, &msg));
+	if (unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
+
+	return 0;
+}
+
 static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
@@ -885,6 +975,7 @@ static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool
 }
 
 static struct remote_subtest tuner_ctl_subtests[] = {
+	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
 	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
 	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
 	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
-- 
2.23.0

Re: [PATCH v4 3/3] cec-compliance: add tuner control test

[Hans Verkuil]

On 10/1/19 5:18 AM, Jiunn Chang wrote:
> Add test for new features added to cec-follower.
> 
> Analog tuner control test tuner_ctl_test():
>   - give analog tuner status
>   - select tuner analog service
>   - analog tuner step features
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp | 91 +++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index aece546c..17f72cc6 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -16,6 +16,7 @@
>  #include <sys/ioctl.h>
>  #include <config.h>
>  #include <sstream>
> +#include <vector>
>  
>  #include "cec-compliance.h"
>  
> @@ -722,6 +723,95 @@ static struct remote_subtest deck_ctl_subtests[] = {
>    TODO: These are very rudimentary tests which should be expanded.
>   */
>  
> +static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
> +{
> +	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	std::vector<struct cec_op_tuner_device_info> info_vec;
> +	__u16 start_freq;
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);

That's interesting! I never noticed this Status Request argument. This is something that
the follower doesn't implement at the moment. It also means that the cec_msg_give_tuner_device_status
function is slightly buggy in that it sets the reply field unconditionally, even though
there will not be a reply if status_req is CEC_OP_STATUS_REQ_OFF.

I'll post a patch for this and cec_msg_give_deck_status which has the same problem.

> +	fail_on_test(!transmit_timeout(node, &msg));
> +	fail_on_test(timed_out(&msg));

Don't fail on this. Instead add to the next if:

> +	if (unrecognized_op(&msg))

if (timed_out(&msg) || unrecognized_op(&msg))

Too many devices time out if they don't support a message rather than
replying with Feature Abort.

> +		return NOTSUPPORTED;
> +	if (refused(&msg))
> +		return REFUSED;
> +	cec_ops_tuner_device_status(&msg, &info);
> +	start_freq = info.analog.ana_freq;
> +	info_vec.push_back(info);
> +
> +	while (1) {
> +		struct cec_op_tuner_device_info new_info;

No need, you can reuse the existing info variable.

> +
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_tuner_step_increment(&msg);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;

This needs a bit more work:

It should fail if the command timed_out. If it returned feature abort, then
if the reason is CEC_OP_ABORT_UNRECOGNIZED_OP it should fail as well.
For any other reason you can assume that the tuner has reached the end of
the service list and you can break out of the loop. I would warn as well in that
case if the abort reason is anything else but REFUSED.

If give_tuner_device_status is working, but not tuner_step_increment, then
that counts as a fail.

> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out(&msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;

Just fail: fail_on_test(timed_out_or_abort(&msg));

It worked above, so if this one fails, then something is seriously wrong.

> +		cec_ops_tuner_device_status(&msg, &new_info);
> +		if (new_info.analog.ana_freq == start_freq)
> +			break;
> +		if (new_info.analog.ana_freq == info_vec.back().analog.ana_freq) {
> +			warn("Tuner channel step increment does not wrap.\n");
> +			break;
> +		}
> +		info_vec.push_back(new_info);
> +	}
> +
> +	
> +	for (std::vector<struct cec_op_tuner_device_info>::iterator iter = info_vec.begin();
> +			iter != info_vec.end(); iter++) {
> +		struct cec_op_tuner_device_info current;

Again, you can just reuse the info variable above.

> +
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_select_analogue_service(&msg, iter->analog.ana_bcast_type,
> +			iter->analog.ana_freq, iter->analog.bcast_system);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;

I think that there is no reason why this would fail, so just do
fail_on_test(timed_out_or_abort(&msg));

In my experience it is usually best to be a bit strict in the tests and relax them
if you find real cases where the strict test would fail.

> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out(&msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;

Definitely a fail.

> +		cec_ops_tuner_device_status(&msg, &current);
> +		fail_on_test(current.analog.ana_freq != iter->analog.ana_freq);
> +		if (iter->is_analog) {
> +			float freq_mhz = (iter->analog.ana_freq * 625) / 10000;

I think this should be 10000.0 to force this division to be a division of floats,
not integers. I'd also use double instead of float.

> +			info("Analog channel freq: %.2f MHz\n", freq_mhz);
> +		}
> +	}
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	if (unrecognized_op(&msg))
> +		return NOTSUPPORTED;
> +	if (refused(&msg))
> +		return REFUSED;

Any time out or unrecognized_op reason is a fail.

> +	fail_on_test(!cec_msg_status_is_abort(&msg));
> +	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
> +
> +	return 0;
> +}
> +
>  static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> @@ -885,6 +975,7 @@ static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool
>  }
>  
>  static struct remote_subtest tuner_ctl_subtests[] = {
> +	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
>  	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
>  	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
>  	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
> 

This looks promising!

Regards,

	Hans

[PATCH v3 1/3] cec-follower: fix bugs for tuner emulation

[Jiunn Chang]

Fix some typos for tuner emulation:
  - analog_tuner_init() store ana_freq in hex
  - analog_get_nearest_freq() temp variable check

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-tuner.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index acc3fd00..2f6e3425 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -92,13 +92,15 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
 
 void analog_tuner_init(struct cec_op_tuner_device_info *info)
 {
+	unsigned int freq_khz =
+		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
+
 	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
 	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
 	info->is_analog = true;
 	info->analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
 	info->analog.bcast_system = CEC_OP_BCAST_SYSTEM_PAL_BG;
-	info->analog.ana_freq =
-		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
+	info->analog.ana_freq = (freq_khz * 10) / 625;
 }
 
 static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
@@ -123,7 +125,7 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	__u8 system;
 
 	cec_ops_select_analogue_service(msg, &type, &freq, &system);
-	if (info->analog.ana_bcast_type < 3 && info->analog.bcast_system < 9) {
+	if (type < 3 && system < 9) {
 		int freq_khz = (freq * 625) / 10;
 		unsigned int nearest = analog_get_nearest_freq(type, system,
 							       freq_khz);
-- 
2.23.0

Re: [PATCH v3 1/3] cec-follower: fix bugs for tuner emulation

[Hans Verkuil]

Hi Jiunn,

On 9/30/19 6:30 AM, Jiunn Chang wrote:
> Fix some typos for tuner emulation:
>   - analog_tuner_init() store ana_freq in hex
>   - analog_get_nearest_freq() temp variable check

The patch is fine, but this commit log is wrong.

"store ana_freq in hex" is meaningless. What you mean is "store ana_freq as multiples of 62.5 kHz"
and the second line gives the wrong function name and doesn't really explain the issue.

Regards,

	Hans

> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-follower/cec-tuner.cpp | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index acc3fd00..2f6e3425 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -92,13 +92,15 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
>  
>  void analog_tuner_init(struct cec_op_tuner_device_info *info)
>  {
> +	unsigned int freq_khz =
> +		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
> +
>  	info->rec_flag = CEC_OP_REC_FLAG_NOT_USED;
>  	info->tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_ANALOGUE;
>  	info->is_analog = true;
>  	info->analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
>  	info->analog.bcast_system = CEC_OP_BCAST_SYSTEM_PAL_BG;
> -	info->analog.ana_freq =
> -		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
> +	info->analog.ana_freq = (freq_khz * 10) / 625;
>  }
>  
>  static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
> @@ -123,7 +125,7 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	__u8 system;
>  
>  	cec_ops_select_analogue_service(msg, &type, &freq, &system);
> -	if (info->analog.ana_bcast_type < 3 && info->analog.bcast_system < 9) {
> +	if (type < 3 && system < 9) {
>  		int freq_khz = (freq * 625) / 10;
>  		unsigned int nearest = analog_get_nearest_freq(type, system,
>  							       freq_khz);
> 

[PATCH v3 2/3] cec-follower: add tuner step increment/decrement

[Jiunn Chang]

Tuner step increment/decrement will select the next highest or next
lowest service frequency.  There are a total of three possible
frequencies given a broadcast type and system for a total of 81 analog
channels.

Opcodes implemented:
  - <Tuner Step Increment>
  - <Tuner Step Decrement>

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-follower.cpp |  2 +-
 utils/cec-follower/cec-follower.h   |  3 +-
 utils/cec-follower/cec-tuner.cpp    | 85 +++++++++++++++++++++++++----
 3 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
index 4243fdd9..8375404e 100644
--- a/utils/cec-follower/cec-follower.cpp
+++ b/utils/cec-follower/cec-follower.cpp
@@ -298,7 +298,7 @@ void state_init(struct node &node)
 	node.state.sac_active = false;
 	node.state.volume = 50;
 	node.state.mute = false;
-	analog_tuner_init(&node.state.tuner_dev_info);
+	analog_tuner_init(&node.state);
 }
 
 int main(int argc, char **argv)
diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
index a53c16fe..d8ac4e77 100644
--- a/utils/cec-follower/cec-follower.h
+++ b/utils/cec-follower/cec-follower.h
@@ -52,6 +52,7 @@ struct state {
 	unsigned rc_press_hold_count;
 	unsigned rc_duration_sum;
 	struct cec_op_tuner_device_info tuner_dev_info;
+	unsigned int freq_idx;
 };
 
 struct node {
@@ -221,7 +222,7 @@ std::string opcode2s(const struct cec_msg *msg);
 void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
 
 // cec-tuner.cpp
-void analog_tuner_init(struct cec_op_tuner_device_info *tuner_dev_info);
+void analog_tuner_init(struct state *state);
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
 
 // CEC processing
diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index 2f6e3425..d3c6f9e8 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -90,8 +90,9 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
 	}
 };
 
-void analog_tuner_init(struct cec_op_tuner_device_info *info)
+void analog_tuner_init(struct state *state)
 {
+	struct cec_op_tuner_device_info *info = &state->tuner_dev_info;
 	unsigned int freq_khz =
 		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
 
@@ -101,20 +102,24 @@ void analog_tuner_init(struct cec_op_tuner_device_info *info)
 	info->analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
 	info->analog.bcast_system = CEC_OP_BCAST_SYSTEM_PAL_BG;
 	info->analog.ana_freq = (freq_khz * 10) / 625;
+	state->freq_idx = 18;
 }
 
-static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
-                                            int ana_freq_khz)
+static void analog_get_nearest_freq_idx(__u8 ana_bcast_type, __u8 ana_bcast_system,
+					int ana_freq_khz, int *nearest, int *idx)
 {
-	int nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
+	*nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
+	int offset = 0;
 
 	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
 		int freq = analog_freqs_khz[ana_bcast_type][ana_bcast_system][i];
 
-		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest))
-			nearest = freq;
+		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - *nearest)) {
+			*nearest = freq;
+			offset++;
+		}
 	}
-	return nearest;
+	*idx = 18 + (NUM_ANALOG_FREQS * ((ana_bcast_type * 9) + ana_bcast_system)) + offset;
 }
 
 static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
@@ -127,8 +132,11 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	cec_ops_select_analogue_service(msg, &type, &freq, &system);
 	if (type < 3 && system < 9) {
 		int freq_khz = (freq * 625) / 10;
-		unsigned int nearest = analog_get_nearest_freq(type, system,
-							       freq_khz);
+		int idx;
+		int nearest;
+
+		analog_get_nearest_freq_idx(type, system, freq_khz, &nearest, &idx);
+		node->state.freq_idx = idx;
 		info->analog.ana_bcast_type = type;
 		info->analog.ana_freq = (nearest * 10) / 625;
 		info->analog.bcast_system = system;
@@ -137,6 +145,42 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	return false;
 }
 
+static void analog_get_type_system_idx(unsigned int freq_idx, int *type, int *system, int *idx)
+{
+	int num_freqs = NUM_ANALOG_FREQS * 9;
+	*type = (freq_idx - 18) / num_freqs;
+	*system = (freq_idx - (18 + num_freqs * *type)) / NUM_ANALOG_FREQS;
+	*idx = freq_idx % NUM_ANALOG_FREQS;
+}
+
+static void reset_tuner_dev_info(struct node *node)
+{
+	/*
+	  There are a total of 18 digital and 81 analog indexes with digital
+	  occurring before analog.  0-17 are digital and 18-98 are analog.
+	 */
+
+	struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+
+	if (node->state.freq_idx < 18) {
+		// digital
+		info->is_analog = false;
+	} else {
+		// analog
+		int type;
+		int system;
+		int idx;
+		int freq_khz;
+
+		analog_get_type_system_idx(node->state.freq_idx, &type, &system, &idx);
+		info->is_analog = true;
+		info->analog.ana_bcast_type = type;
+		info->analog.bcast_system = system;
+		freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][idx];
+		info->analog.ana_freq = (freq_khz * 10) / 625;
+	}
+}
+
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
 {
 	bool is_bcast = cec_msg_is_broadcast(&msg);
@@ -180,12 +224,29 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 
 	case CEC_MSG_SELECT_DIGITAL_SERVICE:
-	case CEC_MSG_TUNER_STEP_DECREMENT:
-	case CEC_MSG_TUNER_STEP_INCREMENT:
-		if (!cec_has_tuner(1 << me))
+	case CEC_MSG_TUNER_STEP_DECREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
 			break;
+
+		if (node->state.freq_idx == 18)
+			node->state.freq_idx = 98;
+		else
+			node->state.freq_idx--;
+		reset_tuner_dev_info(node);
 		return;
+	}
 
+	case CEC_MSG_TUNER_STEP_INCREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
+			break;
+
+		if (node->state.freq_idx == 98)
+			node->state.freq_idx = 18;
+		else
+			node->state.freq_idx++;
+		reset_tuner_dev_info(node);
+		return;
+	}
 
 		/*
 		  One Touch Record
-- 
2.23.0

Re: [PATCH v3 2/3] cec-follower: add tuner step increment/decrement

[Hans Verkuil]

On 9/30/19 6:30 AM, Jiunn Chang wrote:
> Tuner step increment/decrement will select the next highest or next
> lowest service frequency.  There are a total of three possible
> frequencies given a broadcast type and system for a total of 81 analog
> channels.
> 
> Opcodes implemented:
>   - <Tuner Step Increment>
>   - <Tuner Step Decrement>
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-follower/cec-follower.cpp |  2 +-
>  utils/cec-follower/cec-follower.h   |  3 +-
>  utils/cec-follower/cec-tuner.cpp    | 85 +++++++++++++++++++++++++----
>  3 files changed, 76 insertions(+), 14 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-follower.cpp b/utils/cec-follower/cec-follower.cpp
> index 4243fdd9..8375404e 100644
> --- a/utils/cec-follower/cec-follower.cpp
> +++ b/utils/cec-follower/cec-follower.cpp
> @@ -298,7 +298,7 @@ void state_init(struct node &node)
>  	node.state.sac_active = false;
>  	node.state.volume = 50;
>  	node.state.mute = false;
> -	analog_tuner_init(&node.state.tuner_dev_info);
> +	analog_tuner_init(&node.state);
>  }
>  
>  int main(int argc, char **argv)
> diff --git a/utils/cec-follower/cec-follower.h b/utils/cec-follower/cec-follower.h
> index a53c16fe..d8ac4e77 100644
> --- a/utils/cec-follower/cec-follower.h
> +++ b/utils/cec-follower/cec-follower.h
> @@ -52,6 +52,7 @@ struct state {
>  	unsigned rc_press_hold_count;
>  	unsigned rc_duration_sum;
>  	struct cec_op_tuner_device_info tuner_dev_info;
> +	unsigned int freq_idx;
>  };
>  
>  struct node {
> @@ -221,7 +222,7 @@ std::string opcode2s(const struct cec_msg *msg);
>  void sad_encode(const struct short_audio_desc *sad, __u32 *descriptor);
>  
>  // cec-tuner.cpp
> -void analog_tuner_init(struct cec_op_tuner_device_info *tuner_dev_info);
> +void analog_tuner_init(struct state *state);
>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me);
>  
>  // CEC processing
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index 2f6e3425..d3c6f9e8 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -90,8 +90,9 @@ static unsigned int analog_freqs_khz[3][9][NUM_ANALOG_FREQS] =
>  	}
>  };
>  
> -void analog_tuner_init(struct cec_op_tuner_device_info *info)
> +void analog_tuner_init(struct state *state)
>  {
> +	struct cec_op_tuner_device_info *info = &state->tuner_dev_info;
>  	unsigned int freq_khz =
>  		analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][0];
>  
> @@ -101,20 +102,24 @@ void analog_tuner_init(struct cec_op_tuner_device_info *info)
>  	info->analog.ana_bcast_type = CEC_OP_ANA_BCAST_TYPE_CABLE;
>  	info->analog.bcast_system = CEC_OP_BCAST_SYSTEM_PAL_BG;
>  	info->analog.ana_freq = (freq_khz * 10) / 625;
> +	state->freq_idx = 18;

Hmm, see below.

>  }
>  
> -static unsigned int analog_get_nearest_freq(__u8 ana_bcast_type, __u8 ana_bcast_system,
> -                                            int ana_freq_khz)
> +static void analog_get_nearest_freq_idx(__u8 ana_bcast_type, __u8 ana_bcast_system,
> +					int ana_freq_khz, int *nearest, int *idx)

Why not have this function return 'unsigned int' containing the index?

And I see no reason why 'nearest' is in the argument list. AFAICT this can
be entirely local to this function.

>  {
> -	int nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
> +	*nearest = analog_freqs_khz[ana_bcast_type][ana_bcast_system][0];
> +	int offset = 0;
>  
>  	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
>  		int freq = analog_freqs_khz[ana_bcast_type][ana_bcast_system][i];
>  
> -		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - nearest))
> -			nearest = freq;
> +		if (abs(ana_freq_khz - freq) < abs(ana_freq_khz - *nearest)) {
> +			*nearest = freq;
> +			offset++;
> +		}
>  	}
> -	return nearest;
> +	*idx = 18 + (NUM_ANALOG_FREQS * ((ana_bcast_type * 9) + ana_bcast_system)) + offset;

The value 18 is probably the offset for future digital channels. But since those are
not added yet in this patch series, this value is completely magical. Just start at
0, and make this change when support for digital channels is added.

>  }
>  
>  static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
> @@ -127,8 +132,11 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	cec_ops_select_analogue_service(msg, &type, &freq, &system);
>  	if (type < 3 && system < 9) {
>  		int freq_khz = (freq * 625) / 10;
> -		unsigned int nearest = analog_get_nearest_freq(type, system,
> -							       freq_khz);
> +		int idx;

unsigned int.

> +		int nearest;
> +
> +		analog_get_nearest_freq_idx(type, system, freq_khz, &nearest, &idx);
> +		node->state.freq_idx = idx;
>  		info->analog.ana_bcast_type = type;
>  		info->analog.ana_freq = (nearest * 10) / 625;
>  		info->analog.bcast_system = system;

Use analog_get_type_system_idx() to fill in these fields (see below for more info).

> @@ -137,6 +145,42 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	return false;
>  }
>  
> +static void analog_get_type_system_idx(unsigned int freq_idx, int *type, int *system, int *idx)
> +{
> +	int num_freqs = NUM_ANALOG_FREQS * 9;

Add empty line between variable declarations and code.

> +	*type = (freq_idx - 18) / num_freqs;
> +	*system = (freq_idx - (18 + num_freqs * *type)) / NUM_ANALOG_FREQS;
> +	*idx = freq_idx % NUM_ANALOG_FREQS;

The idx value is really not that interesting. Why not return the ana_freq value instead?

So this function would fill in the ana_bcast_type, ana_freq and bcast_system fields
based on the freq_idx.

> +}
> +
> +static void reset_tuner_dev_info(struct node *node)
> +{
> +	/*
> +	  There are a total of 18 digital and 81 analog indexes with digital
> +	  occurring before analog.  0-17 are digital and 18-98 are analog.
> +	 */

Just postpone mentioning digital until this is actually added.

Multiline comment blocks in the kernel coding style use:

/*
 *
 *
 */

> +
> +	struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +
> +	if (node->state.freq_idx < 18) {
> +		// digital
> +		info->is_analog = false;
> +	} else {
> +		// analog
> +		int type;
> +		int system;
> +		int idx;
> +		int freq_khz;
> +
> +		analog_get_type_system_idx(node->state.freq_idx, &type, &system, &idx);
> +		info->is_analog = true;
> +		info->analog.ana_bcast_type = type;
> +		info->analog.bcast_system = system;
> +		freq_khz = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][idx];
> +		info->analog.ana_freq = (freq_khz * 10) / 625;
> +	}
> +}
> +
>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
>  {
>  	bool is_bcast = cec_msg_is_broadcast(&msg);
> @@ -180,12 +224,29 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		return;
>  
>  	case CEC_MSG_SELECT_DIGITAL_SERVICE:
> -	case CEC_MSG_TUNER_STEP_DECREMENT:
> -	case CEC_MSG_TUNER_STEP_INCREMENT:
> -		if (!cec_has_tuner(1 << me))
> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>  			break;
> +
> +		if (node->state.freq_idx == 18)
> +			node->state.freq_idx = 98;

'98' is a magic number: this should be a define instead using ARRAY_SIZE(analog_freqs_khz).

> +		else
> +			node->state.freq_idx--;
> +		reset_tuner_dev_info(node);
>  		return;
> +	}
>  
> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
> +			break;
> +
> +		if (node->state.freq_idx == 98)
> +			node->state.freq_idx = 18;
> +		else
> +			node->state.freq_idx++;
> +		reset_tuner_dev_info(node);
> +		return;
> +	}
>  
>  		/*
>  		  One Touch Record
> 

Regards,

	Hans

[PATCH v3 3/3] cec-compliance: refactor tuner control tests

[Jiunn Chang]

Tests refactored for new features added to cec-collower.

Analog tuner control tests combined into tuner_ctl_test():
  - give analog tuner status
  - select tuner analog service
  - analog tuner step features

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 184 +++++++++---------------------
 1 file changed, 54 insertions(+), 130 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index aece546c..bdeb589f 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -722,9 +722,13 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
-static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	struct cec_op_tuner_device_info data[81] = {};
+	int cnt = 0;
+	__u16 start_freq;
 
 	cec_msg_init(&msg, me, la);
 	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
@@ -734,163 +738,83 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
 		return NOTSUPPORTED;
 	if (refused(&msg))
 		return REFUSED;
-	if (cec_msg_status_is_abort(&msg))
-		return PRESUMED_OK;
-
-	return 0;
-}
+	cec_ops_tuner_device_status(&msg, &info);
+	start_freq = info.analog.ana_freq;
+	data[cnt++] = info;
 
-static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
+	while (1) {
+		struct cec_op_tuner_device_info *new_info =
+			(struct cec_op_tuner_device_info *)malloc(sizeof(struct cec_op_tuner_device_info));
 
-	node->remote[la].bcast_sys = ~0;
-	for (unsigned sys = 0; sys <= 8; sys++) {
 		cec_msg_init(&msg, me, la);
-		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
-						7668, sys); // 479.25 MHz analog frequency
+		cec_msg_tuner_step_increment(&msg);
 		fail_on_test(!transmit_timeout(node, &msg));
 		if (unrecognized_op(&msg))
 			return NOTSUPPORTED;
-		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
-			info("Tuner supports %s, but cannot select that service.\n",
-			     bcast_system2s(sys));
-			node->remote[la].bcast_sys = sys;
-			continue;
+		if (refused(&msg))
+			return REFUSED;
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out(&msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_ops_tuner_device_status(&msg, new_info);
+		if (new_info->analog.ana_freq == start_freq)
+			break;
+		if (new_info->analog.ana_freq == data[cnt - 1].analog.ana_freq) {
+			warn("Tuner channel step increment does not wrap.\n");
+			break;
 		}
-		if (cec_msg_status_is_abort(&msg))
-			continue;
-		info("Tuner supports %s\n", bcast_system2s(sys));
-		node->remote[la].bcast_sys = sys;
+		data[cnt] = *new_info;
+		cnt++;
 	}
 
-	if (node->remote[la].bcast_sys == (__u8)~0)
-		warn("No analog broadcast format supported\n");
-	else
-		return 0;
+	for (size_t i = 0; i < sizeof(data) / sizeof(data[0]); i++) {
+		info = data[i];
+		struct cec_op_tuner_device_info current;
 
-	return PRESUMED_OK;
-}
-
-static int tuner_ctl_sel_digital_service(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
-	struct cec_op_digital_service_id digital_service_id = {};
-
-	digital_service_id.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_CHANNEL;
-	digital_service_id.channel.channel_number_fmt = CEC_OP_CHANNEL_NUMBER_FMT_1_PART;
-	digital_service_id.channel.minor = 1;
-
-	__u8 bcast_systems[] = {
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2,
-		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T,
-	};
-
-	node->remote[la].dig_bcast_sys = ~0;
-	for (unsigned i = 0; i < ARRAY_SIZE(bcast_systems); i++) {
-		__u8 sys = bcast_systems[i];
-
-		digital_service_id.dig_bcast_system = sys;
 		cec_msg_init(&msg, me, la);
-		cec_msg_select_digital_service(&msg, &digital_service_id);
+		cec_msg_select_analogue_service(&msg, info.analog.ana_bcast_type,
+			info.analog.ana_freq, info.analog.bcast_system);
 		fail_on_test(!transmit_timeout(node, &msg));
 		if (unrecognized_op(&msg))
 			return NOTSUPPORTED;
-		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
-			info("Tuner supports %s, but cannot select that service.\n",
-			     dig_bcast_system2s(sys));
-			node->remote[la].dig_bcast_sys = sys;
-			continue;
+		if (refused(&msg))
+			return REFUSED;
+		cec_msg_init(&msg, me, la);
+		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+		fail_on_test(!transmit_timeout(node, &msg));
+		fail_on_test(timed_out(&msg));
+		if (unrecognized_op(&msg))
+			return NOTSUPPORTED;
+		if (refused(&msg))
+			return REFUSED;
+		cec_ops_tuner_device_status(&msg, &current);
+		fail_on_test(current.analog.ana_freq != info.analog.ana_freq);
+		if (info.is_analog) {
+			float freq_mhz = (info.analog.ana_freq * 625) / 10000;
+			info("Analog channel freq: %.2f MHz\n", freq_mhz);
 		}
-		if (cec_msg_status_is_abort(&msg))
-			continue;
-		info("Tuner supports %s\n", dig_bcast_system2s(sys));
-		node->remote[la].dig_bcast_sys = sys;
 	}
 
-	if (node->remote[la].dig_bcast_sys == (__u8)~0)
-		warn("No digital broadcast system supported\n");
-	else
-		return 0;
-
-	return PRESUMED_OK;
-}
-
-static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
-	struct cec_op_tuner_device_info tuner_dev_info = {};
-
-	tuner_dev_info.rec_flag = CEC_OP_REC_FLAG_NOT_USED;
-	tuner_dev_info.tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_NONE;
-	tuner_dev_info.is_analog = false;
-	tuner_dev_info.digital.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_CHANNEL;
-	tuner_dev_info.digital.dig_bcast_system = CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C;
-	tuner_dev_info.digital.channel.channel_number_fmt = CEC_OP_CHANNEL_NUMBER_FMT_1_PART;
-	tuner_dev_info.digital.channel.minor = 1;
-
 	cec_msg_init(&msg, me, la);
-
-	cec_msg_tuner_device_status(&msg, &tuner_dev_info);
+	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
 	fail_on_test(!transmit_timeout(node, &msg));
 	if (unrecognized_op(&msg))
 		return NOTSUPPORTED;
 	if (refused(&msg))
 		return REFUSED;
-	if (cec_msg_status_is_abort(&msg))
-		return PRESUMED_OK;
+	fail_on_test(!cec_msg_status_is_abort(&msg));
+	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
 
 	return 0;
 }
 
-static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
-
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_decrement(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
-}
-
-static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
-{
-	struct cec_msg msg = {};
-
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_increment(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
-}
-
 static struct remote_subtest tuner_ctl_subtests[] = {
-	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
-	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
-	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
-	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
-	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
-	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
+	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
 };
 
 
-- 
2.23.0

Re: [PATCH v3 3/3] cec-compliance: refactor tuner control tests

[Hans Verkuil]

On 9/30/19 6:30 AM, Jiunn Chang wrote:
> Tests refactored for new features added to cec-collower.
> 
> Analog tuner control tests combined into tuner_ctl_test():
>   - give analog tuner status
>   - select tuner analog service
>   - analog tuner step features
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-compliance/cec-test.cpp | 184 +++++++++---------------------
>  1 file changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index aece546c..bdeb589f 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -722,9 +722,13 @@ static struct remote_subtest deck_ctl_subtests[] = {
>    TODO: These are very rudimentary tests which should be expanded.
>   */
>  
> -static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_test(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	struct cec_op_tuner_device_info data[81] = {};

Where does 81 come from? You don't know how many channels there are. Use something like
std::vector instead. That will expand dynamically.

> +	int cnt = 0;
> +	__u16 start_freq;
>  
>  	cec_msg_init(&msg, me, la);
>  	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> @@ -734,163 +738,83 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
>  		return NOTSUPPORTED;
>  	if (refused(&msg))
>  		return REFUSED;
> -	if (cec_msg_status_is_abort(&msg))
> -		return PRESUMED_OK;
> -
> -	return 0;
> -}
> +	cec_ops_tuner_device_status(&msg, &info);
> +	start_freq = info.analog.ana_freq;
> +	data[cnt++] = info;
>  
> -static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned la, bool interactive)
> -{
> -	struct cec_msg msg = {};
> +	while (1) {
> +		struct cec_op_tuner_device_info *new_info =
> +			(struct cec_op_tuner_device_info *)malloc(sizeof(struct cec_op_tuner_device_info));

This can just be a local 'struct cec_op_tuner_device_info new_info;' variable.
No need to allocate it. In fact, you introduce a memory leak here.

>  
> -	node->remote[la].bcast_sys = ~0;
> -	for (unsigned sys = 0; sys <= 8; sys++) {
>  		cec_msg_init(&msg, me, la);
> -		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
> -						7668, sys); // 479.25 MHz analog frequency
> +		cec_msg_tuner_step_increment(&msg);
>  		fail_on_test(!transmit_timeout(node, &msg));
>  		if (unrecognized_op(&msg))
>  			return NOTSUPPORTED;
> -		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> -			info("Tuner supports %s, but cannot select that service.\n",
> -			     bcast_system2s(sys));
> -			node->remote[la].bcast_sys = sys;
> -			continue;
> +		if (refused(&msg))
> +			return REFUSED;
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out(&msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;
> +		cec_ops_tuner_device_status(&msg, new_info);
> +		if (new_info->analog.ana_freq == start_freq)
> +			break;
> +		if (new_info->analog.ana_freq == data[cnt - 1].analog.ana_freq) {
> +			warn("Tuner channel step increment does not wrap.\n");
> +			break;
>  		}
> -		if (cec_msg_status_is_abort(&msg))
> -			continue;
> -		info("Tuner supports %s\n", bcast_system2s(sys));
> -		node->remote[la].bcast_sys = sys;
> +		data[cnt] = *new_info;
> +		cnt++;
>  	}
>  
> -	if (node->remote[la].bcast_sys == (__u8)~0)
> -		warn("No analog broadcast format supported\n");
> -	else
> -		return 0;
> +	for (size_t i = 0; i < sizeof(data) / sizeof(data[0]); i++) {
> +		info = data[i];
> +		struct cec_op_tuner_device_info current;
>  
> -	return PRESUMED_OK;
> -}
> -
> -static int tuner_ctl_sel_digital_service(struct node *node, unsigned me, unsigned la, bool interactive)
> -{
> -	struct cec_msg msg = {};
> -	struct cec_op_digital_service_id digital_service_id = {};
> -
> -	digital_service_id.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_CHANNEL;
> -	digital_service_id.channel.channel_number_fmt = CEC_OP_CHANNEL_NUMBER_FMT_1_PART;
> -	digital_service_id.channel.minor = 1;
> -
> -	__u8 bcast_systems[] = {
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_GEN,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_GEN,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_GEN,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_BS,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_CS,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ARIB_T,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_CABLE,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_SAT,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_ATSC_T,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_S2,
> -		CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_T,
> -	};
> -
> -	node->remote[la].dig_bcast_sys = ~0;
> -	for (unsigned i = 0; i < ARRAY_SIZE(bcast_systems); i++) {
> -		__u8 sys = bcast_systems[i];
> -
> -		digital_service_id.dig_bcast_system = sys;
>  		cec_msg_init(&msg, me, la);
> -		cec_msg_select_digital_service(&msg, &digital_service_id);
> +		cec_msg_select_analogue_service(&msg, info.analog.ana_bcast_type,
> +			info.analog.ana_freq, info.analog.bcast_system);
>  		fail_on_test(!transmit_timeout(node, &msg));
>  		if (unrecognized_op(&msg))
>  			return NOTSUPPORTED;
> -		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> -			info("Tuner supports %s, but cannot select that service.\n",
> -			     dig_bcast_system2s(sys));
> -			node->remote[la].dig_bcast_sys = sys;
> -			continue;
> +		if (refused(&msg))
> +			return REFUSED;
> +		cec_msg_init(&msg, me, la);
> +		cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +		fail_on_test(!transmit_timeout(node, &msg));
> +		fail_on_test(timed_out(&msg));
> +		if (unrecognized_op(&msg))
> +			return NOTSUPPORTED;
> +		if (refused(&msg))
> +			return REFUSED;
> +		cec_ops_tuner_device_status(&msg, &current);
> +		fail_on_test(current.analog.ana_freq != info.analog.ana_freq);
> +		if (info.is_analog) {
> +			float freq_mhz = (info.analog.ana_freq * 625) / 10000;
> +			info("Analog channel freq: %.2f MHz\n", freq_mhz);
>  		}
> -		if (cec_msg_status_is_abort(&msg))
> -			continue;
> -		info("Tuner supports %s\n", dig_bcast_system2s(sys));
> -		node->remote[la].dig_bcast_sys = sys;
>  	}
>  
> -	if (node->remote[la].dig_bcast_sys == (__u8)~0)
> -		warn("No digital broadcast system supported\n");
> -	else
> -		return 0;
> -
> -	return PRESUMED_OK;
> -}
> -
> -static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned la, bool interactive)
> -{
> -	struct cec_msg msg = {};
> -	struct cec_op_tuner_device_info tuner_dev_info = {};
> -
> -	tuner_dev_info.rec_flag = CEC_OP_REC_FLAG_NOT_USED;
> -	tuner_dev_info.tuner_display_info = CEC_OP_TUNER_DISPLAY_INFO_NONE;
> -	tuner_dev_info.is_analog = false;
> -	tuner_dev_info.digital.service_id_method = CEC_OP_SERVICE_ID_METHOD_BY_CHANNEL;
> -	tuner_dev_info.digital.dig_bcast_system = CEC_OP_DIG_SERVICE_BCAST_SYSTEM_DVB_C;
> -	tuner_dev_info.digital.channel.channel_number_fmt = CEC_OP_CHANNEL_NUMBER_FMT_1_PART;
> -	tuner_dev_info.digital.channel.minor = 1;
> -
>  	cec_msg_init(&msg, me, la);
> -
> -	cec_msg_tuner_device_status(&msg, &tuner_dev_info);
> +	cec_msg_select_analogue_service(&msg, 3, 16000, 9);
>  	fail_on_test(!transmit_timeout(node, &msg));
>  	if (unrecognized_op(&msg))
>  		return NOTSUPPORTED;
>  	if (refused(&msg))
>  		return REFUSED;
> -	if (cec_msg_status_is_abort(&msg))
> -		return PRESUMED_OK;
> +	fail_on_test(!cec_msg_status_is_abort(&msg));
> +	fail_on_test(abort_reason(&msg) != CEC_OP_ABORT_INVALID_OP);
>  
>  	return 0;
>  }
>  
> -static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
> -{
> -	struct cec_msg msg = {};
> -
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_decrement(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> -}
> -
> -static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
> -{
> -	struct cec_msg msg = {};
> -
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_increment(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> -}
> -
>  static struct remote_subtest tuner_ctl_subtests[] = {
> -	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
> -	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
> -	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
> -	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
> -	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
> -	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
> +	{ "Tuner Control", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_test },
>  };
>  
>  
> 

Hmm, this is too hard to review.

Let's do this a bit differently: keep the existing tests for now and just add the new
Tuner Control test. That way this patch just shows the new function, which makes it
much easier to review. Once we're happy with the new test function, a final patch
can be applied removing the old tests.

I should have realized that, sorry for the additional work.

Regards,

	Hans

[PATCH v2 1/2] cec-follower: add tuner step increment/decrement

[Jiunn Chang]

Tuner step increment/decrement will select the next highest or next
lowest service frequency.  There are a total of three possible
frequencies from analog_freqs_khz given a broadcast type and system.

Opcodes implemented:
  - <Tuner Step Increment>
  - <Tuner Step Decrement>

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-follower/cec-tuner.cpp | 47 ++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
index acc3fd00..1a9b9d90 100644
--- a/utils/cec-follower/cec-tuner.cpp
+++ b/utils/cec-follower/cec-tuner.cpp
@@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
 	return false;
 }
 
+static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
+{
+	if (info->analog.ana_freq == 0)
+		return -1;
+
+	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
+
+	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
+		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
+
+		if (ana_freq_khz == freq)
+			return i;
+	}
+	return -1;
+}
+
 void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
 {
 	bool is_bcast = cec_msg_is_broadcast(&msg);
@@ -178,12 +194,37 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
 		return;
 
 	case CEC_MSG_SELECT_DIGITAL_SERVICE:
-	case CEC_MSG_TUNER_STEP_DECREMENT:
-	case CEC_MSG_TUNER_STEP_INCREMENT:
-		if (!cec_has_tuner(1 << me))
+	case CEC_MSG_TUNER_STEP_DECREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
 			break;
+
+		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+		int freq_idx = analog_find_freq_index(info);
+		int idx = (freq_idx == 0) ? NUM_ANALOG_FREQS : freq_idx;
+
+		if (info->is_analog) {
+			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
+
+			info->analog.ana_freq = (freq * 10) / 625;
+		}
 		return;
+	}
+
+	case CEC_MSG_TUNER_STEP_INCREMENT: {
+		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
+			break;
 
+		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
+		int freq_idx = analog_find_freq_index(info);
+		int idx = (freq_idx == NUM_ANALOG_FREQS - 1) ? -1 : freq_idx;
+
+		if (info->is_analog) {
+			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
+
+			info->analog.ana_freq = (freq * 10) / 625;
+		}
+		return;
+	}
 
 		/*
 		  One Touch Record
-- 
2.23.0

Re: [Linux-kernel-mentees] [PATCH v2 1/2] cec-follower: add tuner step increment/decrement

[Shuah Khan]

On 9/24/19 1:24 PM, Jiunn Chang wrote:
> Tuner step increment/decrement will select the next highest or next
> lowest service frequency.  There are a total of three possible
> frequencies from analog_freqs_khz given a broadcast type and system.
> 
> Opcodes implemented:
>    - <Tuner Step Increment>
>    - <Tuner Step Decrement>
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>   utils/cec-follower/cec-tuner.cpp | 47 ++++++++++++++++++++++++++++++--
>   1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index acc3fd00..1a9b9d90 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>   	return false;
>   }
>   
> +static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
> +{
> +	if (info->analog.ana_freq == 0)
> +		return -1;
> +
> +	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
> +
> +	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
> +		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
> +
> +		if (ana_freq_khz == freq)
> +			return i;
> +	}
> +	return -1;
> +}
> +
>   void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
>   {
>   	bool is_bcast = cec_msg_is_broadcast(&msg);
> @@ -178,12 +194,37 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>   		return;
>   
>   	case CEC_MSG_SELECT_DIGITAL_SERVICE:
> -	case CEC_MSG_TUNER_STEP_DECREMENT:
> -	case CEC_MSG_TUNER_STEP_INCREMENT:
> -		if (!cec_has_tuner(1 << me))
> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>   			break;
> +
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int freq_idx = analog_find_freq_index(info);
> +		int idx = (freq_idx == 0) ? NUM_ANALOG_FREQS : freq_idx;
> +
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
> +

Do you need this extra line.

> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
>   		return;
> +	}
> +
> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
> +			break;
>   
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int freq_idx = analog_find_freq_index(info);
> +		int idx = (freq_idx == NUM_ANALOG_FREQS - 1) ? -1 : freq_idx;
> +
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
> +
Do you need this extra line.

> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
> +		return;
> +	}
>   
>   		/*
>   		  One Touch Record
> 

thanks,
-- Shuah

Re: [PATCH v2 1/2] cec-follower: add tuner step increment/decrement

[Hans Verkuil]

Hi Jiunn,

On 9/24/19 9:24 PM, Jiunn Chang wrote:
> Tuner step increment/decrement will select the next highest or next
> lowest service frequency.  There are a total of three possible
> frequencies from analog_freqs_khz given a broadcast type and system.
> 
> Opcodes implemented:
>   - <Tuner Step Increment>
>   - <Tuner Step Decrement>
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>  utils/cec-follower/cec-tuner.cpp | 47 ++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
> index acc3fd00..1a9b9d90 100644
> --- a/utils/cec-follower/cec-tuner.cpp
> +++ b/utils/cec-follower/cec-tuner.cpp
> @@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>  	return false;
>  }
>  
> +static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
> +{
> +	if (info->analog.ana_freq == 0)
> +		return -1;
> +
> +	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
> +
> +	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
> +		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
> +
> +		if (ana_freq_khz == freq)
> +			return i;
> +	}
> +	return -1;
> +}

Hmm, I don't like this code. I think that it would be cleaner to just add:

static unsigned int analog_freq_idx[3][9];

Which contains the index of the current frequency for each bcast_type/system
combination.

This ensures that 1) there always is a current frequency and 2) tuner stepping
is now trivial since all you need to do is to update the corresponding index.

Now, that said, I was thinking that it would be nice to have one bcast_type/system
combo with no channels at all, since that's a nice test as well.

I think that setting secam-lq for satellite to { 0, 0, 0 } to indicate no
defined channels would make a nice test case.

Something for a follow-up patch, though.

> +
>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
>  {
>  	bool is_bcast = cec_msg_is_broadcast(&msg);
> @@ -178,12 +194,37 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>  		return;
>  
>  	case CEC_MSG_SELECT_DIGITAL_SERVICE:
> -	case CEC_MSG_TUNER_STEP_DECREMENT:
> -	case CEC_MSG_TUNER_STEP_INCREMENT:
> -		if (!cec_has_tuner(1 << me))
> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>  			break;
> +
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int freq_idx = analog_find_freq_index(info);
> +		int idx = (freq_idx == 0) ? NUM_ANALOG_FREQS : freq_idx;
> +
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
> +
> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
>  		return;
> +	}
> +
> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
> +			break;
>  
> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
> +		int freq_idx = analog_find_freq_index(info);
> +		int idx = (freq_idx == NUM_ANALOG_FREQS - 1) ? -1 : freq_idx;
> +
> +		if (info->is_analog) {
> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
> +
> +			info->analog.ana_freq = (freq * 10) / 625;
> +		}
> +		return;
> +	}
>  
>  		/*
>  		  One Touch Record
> 

Regards,

	Hans

Re: [PATCH v2 1/2] cec-follower: add tuner step increment/decrement

[Hans Verkuil]

On 9/25/19 8:54 AM, Hans Verkuil wrote:
> Hi Jiunn,
> 
> On 9/24/19 9:24 PM, Jiunn Chang wrote:
>> Tuner step increment/decrement will select the next highest or next
>> lowest service frequency.  There are a total of three possible
>> frequencies from analog_freqs_khz given a broadcast type and system.
>>
>> Opcodes implemented:
>>   - <Tuner Step Increment>
>>   - <Tuner Step Decrement>
>>
>> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
>> ---
>>  utils/cec-follower/cec-tuner.cpp | 47 ++++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/cec-follower/cec-tuner.cpp b/utils/cec-follower/cec-tuner.cpp
>> index acc3fd00..1a9b9d90 100644
>> --- a/utils/cec-follower/cec-tuner.cpp
>> +++ b/utils/cec-follower/cec-tuner.cpp
>> @@ -135,6 +135,22 @@ static bool analog_set_tuner_dev_info(struct node *node, struct cec_msg *msg)
>>  	return false;
>>  }
>>  
>> +static int analog_find_freq_index(struct cec_op_tuner_device_info *info)
>> +{
>> +	if (info->analog.ana_freq == 0)
>> +		return -1;
>> +
>> +	int ana_freq_khz = (info->analog.ana_freq * 625) / 10;
>> +
>> +	for (int i = 0; i < NUM_ANALOG_FREQS; i++) {
>> +		int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][i];
>> +
>> +		if (ana_freq_khz == freq)
>> +			return i;
>> +	}
>> +	return -1;
>> +}
> 
> Hmm, I don't like this code. I think that it would be cleaner to just add:
> 
> static unsigned int analog_freq_idx[3][9];
> 
> Which contains the index of the current frequency for each bcast_type/system
> combination.
> 
> This ensures that 1) there always is a current frequency and 2) tuner stepping
> is now trivial since all you need to do is to update the corresponding index.

I've been reading up on this a bit more, and section CEC 13.8.2 states that the
Tuner Step commands cycle through a set of services defined by the tuner device.
But normally it will cycle through all services.

That means that rather than cycling through the frequencies of just the current
bcast_type/system, the follower should cycle through ALL channels, both analog
and digital.

So for analog we really have 3 * 9 * 3 channels, and this will be more once the
digital channels are added.

In any case, this means that there is just a single 'static unsigned int freq_idx;'
and not an array.

Regards,

	Hans

> 
> Now, that said, I was thinking that it would be nice to have one bcast_type/system
> combo with no channels at all, since that's a nice test as well.
> 
> I think that setting secam-lq for satellite to { 0, 0, 0 } to indicate no
> defined channels would make a nice test case.
> 
> Something for a follow-up patch, though.
> 
>> +
>>  void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, unsigned me)
>>  {
>>  	bool is_bcast = cec_msg_is_broadcast(&msg);
>> @@ -178,12 +194,37 @@ void process_tuner_record_timer_msgs(struct node *node, struct cec_msg &msg, uns
>>  		return;
>>  
>>  	case CEC_MSG_SELECT_DIGITAL_SERVICE:
>> -	case CEC_MSG_TUNER_STEP_DECREMENT:
>> -	case CEC_MSG_TUNER_STEP_INCREMENT:
>> -		if (!cec_has_tuner(1 << me))
>> +	case CEC_MSG_TUNER_STEP_DECREMENT: {
>> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>>  			break;
>> +
>> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
>> +		int freq_idx = analog_find_freq_index(info);
>> +		int idx = (freq_idx == 0) ? NUM_ANALOG_FREQS : freq_idx;
>> +
>> +		if (info->is_analog) {
>> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][--idx];
>> +
>> +			info->analog.ana_freq = (freq * 10) / 625;
>> +		}
>>  		return;
>> +	}
>> +
>> +	case CEC_MSG_TUNER_STEP_INCREMENT: {
>> +		if (!cec_has_tuner(1 << me) && !cec_has_tv(1 << me))
>> +			break;
>>  
>> +		struct cec_op_tuner_device_info *info = &node->state.tuner_dev_info;
>> +		int freq_idx = analog_find_freq_index(info);
>> +		int idx = (freq_idx == NUM_ANALOG_FREQS - 1) ? -1 : freq_idx;
>> +
>> +		if (info->is_analog) {
>> +			int freq = analog_freqs_khz[info->analog.ana_bcast_type][info->analog.bcast_system][++idx];
>> +
>> +			info->analog.ana_freq = (freq * 10) / 625;
>> +		}
>> +		return;
>> +	}
>>  
>>  		/*
>>  		  One Touch Record
>>
> 
> Regards,
> 
> 	Hans
> 

[PATCH v2 2/2] cec-compliance: add/refactor tuner control tests

[Jiunn Chang]

Tests added/refactored for new features added to the cec-follower.

Analog tuner control tests added/refactored:
  - give analog tuner status
  - select tuner analog service
  - analog tuner step decrement
  - analog tuner step increment

Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
---
 utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
 1 file changed, 140 insertions(+), 41 deletions(-)

diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
index aece546c..91600d39 100644
--- a/utils/cec-compliance/cec-test.cpp
+++ b/utils/cec-compliance/cec-test.cpp
@@ -722,9 +722,19 @@ static struct remote_subtest deck_ctl_subtests[] = {
   TODO: These are very rudimentary tests which should be expanded.
  */
 
-static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+
+	cec_msg_init(&msg, me, la);
+	cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
+			                7668, CEC_OP_BCAST_SYSTEM_PAL_BG); // 479.25 MHz analog frequency
+	fail_on_test(!transmit_timeout(node, &msg));
+	if (unrecognized_op(&msg))
+		return NOTSUPPORTED;
+	if (refused(&msg))
+		return REFUSED;
 
 	cec_msg_init(&msg, me, la);
 	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
@@ -737,6 +747,14 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
 	if (cec_msg_status_is_abort(&msg))
 		return PRESUMED_OK;
 
+	cec_ops_tuner_device_status(&msg, &info);
+	if (info.analog.ana_bcast_type != CEC_OP_ANA_BCAST_TYPE_CABLE)
+		return FAIL;
+	if (info.analog.ana_freq != 7668)
+		return FAIL;
+	if (info.analog.bcast_system != CEC_OP_BCAST_SYSTEM_PAL_BG)
+		return FAIL;
+
 	return 0;
 }
 
@@ -745,23 +763,24 @@ static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned
 	struct cec_msg msg = {};
 
 	node->remote[la].bcast_sys = ~0;
-	for (unsigned sys = 0; sys <= 8; sys++) {
-		cec_msg_init(&msg, me, la);
-		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
-						7668, sys); // 479.25 MHz analog frequency
-		fail_on_test(!transmit_timeout(node, &msg));
-		if (unrecognized_op(&msg))
-			return NOTSUPPORTED;
-		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
-			info("Tuner supports %s, but cannot select that service.\n",
-			     bcast_system2s(sys));
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 7668, sys); // 479.25 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
+				info("Tuner supports %s, but cannot select that service.\n",
+				bcast_system2s(sys));
+				node->remote[la].bcast_sys = sys;
+				continue;
+			}
+			if (cec_msg_status_is_abort(&msg))
+				continue;
+			info("Tuner supports %s\n", bcast_system2s(sys));
 			node->remote[la].bcast_sys = sys;
-			continue;
 		}
-		if (cec_msg_status_is_abort(&msg))
-			continue;
-		info("Tuner supports %s\n", bcast_system2s(sys));
-		node->remote[la].bcast_sys = sys;
 	}
 
 	if (node->remote[la].bcast_sys == (__u8)~0)
@@ -854,43 +873,123 @@ static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned l
 	return 0;
 }
 
-static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	__u16 freq = 0;
+
+	info.is_analog = true;
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 16000, sys); // 1000 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			freq = info.analog.ana_freq;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_tuner_step_decrement(&msg);
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			if (cec_msg_status_is_abort(&msg))
+				return PRESUMED_OK;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			if (!(info.analog.ana_freq < freq))
+				return FAIL;
+		}
+	}
 
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_decrement(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
+	return 0;
 }
 
-static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
+static int tuner_ctl_analog_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
 {
 	struct cec_msg msg = {};
+	struct cec_op_tuner_device_info info = {};
+	__u16 freq = 0;
+
+	info.is_analog = true;
+	for (unsigned type = 0; type < 3; type++) {
+		for (unsigned sys = 0; sys < 9; sys++) {
+			cec_msg_init(&msg, me, la);
+			cec_msg_select_analogue_service(&msg, type, 0, sys); // 0 MHz analog frequency
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			freq = info.analog.ana_freq;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_tuner_step_increment(&msg);
+			fail_on_test(!transmit_timeout(node, &msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			if (cec_msg_status_is_abort(&msg))
+				return PRESUMED_OK;
+
+			cec_msg_init(&msg, me, la);
+			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
+			fail_on_test(!transmit_timeout(node, &msg));
+			fail_on_test(timed_out(&msg));
+			if (unrecognized_op(&msg))
+				return NOTSUPPORTED;
+			if (refused(&msg))
+				return REFUSED;
+			cec_ops_tuner_device_status(&msg, &info);
+			if (!(info.analog.ana_freq > freq))
+				return FAIL;
+		}
+	}
 
-	cec_msg_init(&msg, me, la);
-	cec_msg_tuner_step_increment(&msg);
-	fail_on_test(!transmit_timeout(node, &msg));
-	if (unrecognized_op(&msg))
-		return NOTSUPPORTED;
-	if (refused(&msg))
-		return REFUSED;
-
-	return PRESUMED_OK;
+	return 0;
 }
 
 static struct remote_subtest tuner_ctl_subtests[] = {
-	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
-	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
-	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
+	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_give_status },
+	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_analog_service },
+	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_digital_service },
 	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
-	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
-	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
+	{ "Tuner Analog Step Decrement", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_dec },
+	{ "Tuner Analog Step Increment", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_inc },
 };
 
 
-- 
2.23.0

Re: [Linux-kernel-mentees] [PATCH v2 2/2] cec-compliance: add/refactor tuner control tests

[Shuah Khan]

On 9/24/19 1:24 PM, Jiunn Chang wrote:
> Tests added/refactored for new features added to the cec-follower.
> 
> Analog tuner control tests added/refactored:
>    - give analog tuner status
>    - select tuner analog service
>    - analog tuner step decrement
>    - analog tuner step increment
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> ---
>   utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
>   1 file changed, 140 insertions(+), 41 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index aece546c..91600d39 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -722,9 +722,19 @@ static struct remote_subtest deck_ctl_subtests[] = {
>     TODO: These are very rudimentary tests which should be expanded.
>    */
>   
> -static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
>   {
>   	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
> +			                7668, CEC_OP_BCAST_SYSTEM_PAL_BG); // 479.25 MHz analog frequency
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	if (unrecognized_op(&msg))
> +		return NOTSUPPORTED;
> +	if (refused(&msg))
> +		return REFUSED;
>   
>   	cec_msg_init(&msg, me, la);
>   	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> @@ -737,6 +747,14 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
>   	if (cec_msg_status_is_abort(&msg))
>   		return PRESUMED_OK;
>   
> +	cec_ops_tuner_device_status(&msg, &info);
> +	if (info.analog.ana_bcast_type != CEC_OP_ANA_BCAST_TYPE_CABLE)
> +		return FAIL;
> +	if (info.analog.ana_freq != 7668)
> +		return FAIL;
> +	if (info.analog.bcast_system != CEC_OP_BCAST_SYSTEM_PAL_BG)
> +		return FAIL;
> +

Why not combine these conditionals in to one. Why do you need 3 separate
blocks?

>   	return 0;
>   }
>   
> @@ -745,23 +763,24 @@ static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned
>   	struct cec_msg msg = {};
>   
>   	node->remote[la].bcast_sys = ~0;
> -	for (unsigned sys = 0; sys <= 8; sys++) {
> -		cec_msg_init(&msg, me, la);
> -		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
> -						7668, sys); // 479.25 MHz analog frequency
> -		fail_on_test(!transmit_timeout(node, &msg));
> -		if (unrecognized_op(&msg))
> -			return NOTSUPPORTED;
> -		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> -			info("Tuner supports %s, but cannot select that service.\n",
> -			     bcast_system2s(sys));
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 7668, sys); // 479.25 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));

Adding line here will help readability.

> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;

Same here.

> +			if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> +				info("Tuner supports %s, but cannot select that service.\n",
> +				bcast_system2s(sys));
> +				node->remote[la].bcast_sys = sys;
> +				continue;
> +			}
same here.

> +			if (cec_msg_status_is_abort(&msg))
> +				continue;
> +			info("Tuner supports %s\n", bcast_system2s(sys));
>   			node->remote[la].bcast_sys = sys;
> -			continue;
>   		}
> -		if (cec_msg_status_is_abort(&msg))
> -			continue;
> -		info("Tuner supports %s\n", bcast_system2s(sys));
> -		node->remote[la].bcast_sys = sys;
>   	}
>   
>   	if (node->remote[la].bcast_sys == (__u8)~0)
> @@ -854,43 +873,123 @@ static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned l
>   	return 0;
>   }
>   
> -static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
>   {
>   	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	__u16 freq = 0;
> +
> +	info.is_analog = true;
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 16000, sys); // 1000 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;

Add a goto NOTSUPPORTED; Makes it easier.
You can simplify this logic a lot.

> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			freq = info.analog.ana_freq;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_tuner_step_decrement(&msg);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			if (cec_msg_status_is_abort(&msg))
> +				return PRESUMED_OK;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			if (!(info.analog.ana_freq < freq))
> +				return FAIL;
> +		}
> +	}
>   
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_decrement(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> +	return 0;
>   }
>   
> -static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
>   {
>   	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	__u16 freq = 0;
> +
> +	info.is_analog = true;
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 0, sys); // 0 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;

Adding blank lines will improve readability.

> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			freq = info.analog.ana_freq;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_tuner_step_increment(&msg);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;

Same here.

> +			if (refused(&msg))
> +				return REFUSED;
> +			if (cec_msg_status_is_abort(&msg))
> +				return PRESUMED_OK;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			if (!(info.analog.ana_freq > freq))
> +				return FAIL;
> +		}
> +	}
>  
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_increment(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> +	return 0;
>   }

tuner_ctl_analog_step_inc() and tuner_ctl_analog_step_dec() has lots of
common code. The only real difference is calls to 
cec_msg_tuner_step_increment() vs. cec_msg_tuner_step_decrement()

Also the error logic is very hard to read.

>   
>   static struct remote_subtest tuner_ctl_subtests[] = {
> -	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
> -	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
> -	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
> +	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_give_status },
> +	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_analog_service },
> +	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_digital_service },
>   	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
> -	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
> -	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
> +	{ "Tuner Analog Step Decrement", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_dec },
> +	{ "Tuner Analog Step Increment", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_inc },
>   };
>   
>   
> 

thanks,
-- Shuah

Re: [PATCH v2 2/2] cec-compliance: add/refactor tuner control tests

[Hans Verkuil]

Hi Jiunn,

On 9/24/19 9:24 PM, Jiunn Chang wrote:
> Tests added/refactored for new features added to the cec-follower.
> 
> Analog tuner control tests added/refactored:
>   - give analog tuner status
>   - select tuner analog service
>   - analog tuner step decrement
>   - analog tuner step increment
> 
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>

I've been thinking about how to test this and I think this needs to be
redesigned. Right now we have little tests for each CEC message, but
it's all too uncoordinated.

I think we should have a single tuner_ctl_test() function that tests the
whole Tuner Control feature.

Basically it should:

1) Test Give Tuner Device Status and store the returned channel to a
   data structure
2) Call Tuner Step Increment repeatedly followed by Give Tuner Device Status
   and store the new channels into the data struct until the Tuner Step Increment
   either returns with an already found channel (it cycled back), or possibly
   failed if it reached the end of the channel list (then it doesn't cycle. Sadly
   the spec is unclear whether the Tuner Step commands should wrap around or not).
   If it failed, then call Select Analog/Digital Service with the initial channel
   found in 1) and call Tuner Step Decrement to find the remaining channel.
3) For all channels in the data structure call Select Analog/Digital Service and
   check with Give Tuner Device Status that it succeeded.
4) Test Select Analog/Digital Service as well with an invalid service (i.e., one
   that was not found in the channel search).

This tests everything in one coherent test.

In addition, this tests what channels are available. If --verbose was given, then
you want to log each found channel.

Regards,

	Hans

> ---
>  utils/cec-compliance/cec-test.cpp | 181 +++++++++++++++++++++++-------
>  1 file changed, 140 insertions(+), 41 deletions(-)
> 
> diff --git a/utils/cec-compliance/cec-test.cpp b/utils/cec-compliance/cec-test.cpp
> index aece546c..91600d39 100644
> --- a/utils/cec-compliance/cec-test.cpp
> +++ b/utils/cec-compliance/cec-test.cpp
> @@ -722,9 +722,19 @@ static struct remote_subtest deck_ctl_subtests[] = {
>    TODO: These are very rudimentary tests which should be expanded.
>   */
>  
> -static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_give_status(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +
> +	cec_msg_init(&msg, me, la);
> +	cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
> +			                7668, CEC_OP_BCAST_SYSTEM_PAL_BG); // 479.25 MHz analog frequency
> +	fail_on_test(!transmit_timeout(node, &msg));
> +	if (unrecognized_op(&msg))
> +		return NOTSUPPORTED;
> +	if (refused(&msg))
> +		return REFUSED;
>  
>  	cec_msg_init(&msg, me, la);
>  	cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> @@ -737,6 +747,14 @@ static int tuner_ctl_give_status(struct node *node, unsigned me, unsigned la, bo
>  	if (cec_msg_status_is_abort(&msg))
>  		return PRESUMED_OK;
>  
> +	cec_ops_tuner_device_status(&msg, &info);
> +	if (info.analog.ana_bcast_type != CEC_OP_ANA_BCAST_TYPE_CABLE)
> +		return FAIL;
> +	if (info.analog.ana_freq != 7668)
> +		return FAIL;
> +	if (info.analog.bcast_system != CEC_OP_BCAST_SYSTEM_PAL_BG)
> +		return FAIL;
> +
>  	return 0;
>  }
>  
> @@ -745,23 +763,24 @@ static int tuner_ctl_sel_analog_service(struct node *node, unsigned me, unsigned
>  	struct cec_msg msg = {};
>  
>  	node->remote[la].bcast_sys = ~0;
> -	for (unsigned sys = 0; sys <= 8; sys++) {
> -		cec_msg_init(&msg, me, la);
> -		cec_msg_select_analogue_service(&msg, CEC_OP_ANA_BCAST_TYPE_CABLE,
> -						7668, sys); // 479.25 MHz analog frequency
> -		fail_on_test(!transmit_timeout(node, &msg));
> -		if (unrecognized_op(&msg))
> -			return NOTSUPPORTED;
> -		if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> -			info("Tuner supports %s, but cannot select that service.\n",
> -			     bcast_system2s(sys));
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 7668, sys); // 479.25 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (abort_reason(&msg) == CEC_OP_ABORT_INVALID_OP) {
> +				info("Tuner supports %s, but cannot select that service.\n",
> +				bcast_system2s(sys));
> +				node->remote[la].bcast_sys = sys;
> +				continue;
> +			}
> +			if (cec_msg_status_is_abort(&msg))
> +				continue;
> +			info("Tuner supports %s\n", bcast_system2s(sys));
>  			node->remote[la].bcast_sys = sys;
> -			continue;
>  		}
> -		if (cec_msg_status_is_abort(&msg))
> -			continue;
> -		info("Tuner supports %s\n", bcast_system2s(sys));
> -		node->remote[la].bcast_sys = sys;
>  	}
>  
>  	if (node->remote[la].bcast_sys == (__u8)~0)
> @@ -854,43 +873,123 @@ static int tuner_ctl_tuner_dev_status(struct node *node, unsigned me, unsigned l
>  	return 0;
>  }
>  
> -static int tuner_ctl_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_step_dec(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	__u16 freq = 0;
> +
> +	info.is_analog = true;
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 16000, sys); // 1000 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			freq = info.analog.ana_freq;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_tuner_step_decrement(&msg);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			if (cec_msg_status_is_abort(&msg))
> +				return PRESUMED_OK;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			if (!(info.analog.ana_freq < freq))
> +				return FAIL;
> +		}
> +	}
>  
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_decrement(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> +	return 0;
>  }
>  
> -static int tuner_ctl_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
> +static int tuner_ctl_analog_step_inc(struct node *node, unsigned me, unsigned la, bool interactive)
>  {
>  	struct cec_msg msg = {};
> +	struct cec_op_tuner_device_info info = {};
> +	__u16 freq = 0;
> +
> +	info.is_analog = true;
> +	for (unsigned type = 0; type < 3; type++) {
> +		for (unsigned sys = 0; sys < 9; sys++) {
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_select_analogue_service(&msg, type, 0, sys); // 0 MHz analog frequency
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			freq = info.analog.ana_freq;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_tuner_step_increment(&msg);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			if (cec_msg_status_is_abort(&msg))
> +				return PRESUMED_OK;
> +
> +			cec_msg_init(&msg, me, la);
> +			cec_msg_give_tuner_device_status(&msg, true, CEC_OP_STATUS_REQ_ONCE);
> +			fail_on_test(!transmit_timeout(node, &msg));
> +			fail_on_test(timed_out(&msg));
> +			if (unrecognized_op(&msg))
> +				return NOTSUPPORTED;
> +			if (refused(&msg))
> +				return REFUSED;
> +			cec_ops_tuner_device_status(&msg, &info);
> +			if (!(info.analog.ana_freq > freq))
> +				return FAIL;
> +		}
> +	}
>  
> -	cec_msg_init(&msg, me, la);
> -	cec_msg_tuner_step_increment(&msg);
> -	fail_on_test(!transmit_timeout(node, &msg));
> -	if (unrecognized_op(&msg))
> -		return NOTSUPPORTED;
> -	if (refused(&msg))
> -		return REFUSED;
> -
> -	return PRESUMED_OK;
> +	return 0;
>  }
>  
>  static struct remote_subtest tuner_ctl_subtests[] = {
> -	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_give_status },
> -	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_analog_service },
> -	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_sel_digital_service },
> +	{ "Give Tuner Device Status", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_give_status },
> +	{ "Select Analogue Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_analog_service },
> +	{ "Select Digital Service", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_sel_digital_service },
>  	{ "Tuner Device Status", CEC_LOG_ADDR_MASK_ALL, tuner_ctl_tuner_dev_status },
> -	{ "Tuner Step Decrement", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_dec },
> -	{ "Tuner Step Increment", CEC_LOG_ADDR_MASK_TUNER, tuner_ctl_step_inc },
> +	{ "Tuner Analog Step Decrement", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_dec },
> +	{ "Tuner Analog Step Increment", CEC_LOG_ADDR_MASK_TUNER | CEC_LOG_ADDR_MASK_TV, tuner_ctl_analog_step_inc },
>  };
>  
>  
>