cecd-devel

[cecd-devel] [PATCH 0/6] cecd and libcec compliance fixes

[Florian F. <f.f...@gm...>]

All of these patches are related to the compliance testing I performed
using both our own implementation and cecd. They should make CEC devices
conform better to the HDMI CEC CTS 1.3.

Florian Fainelli (6):
  [cecd] answer to <Abort> opcode
  [cecd] check and handle libcec_decode_message return values
  [libcec] compute src and dst once and for all
  [libcec] check unicast/broadcast flag with destination address
  [cecd] handle LIBCEC_ERROR_OTHER from libcec_decode_message
  [cecd] fix BROADCAST usage

 cecd/cecd.c      |   39 ++++++++++++++++++++++++++++++++++-----
 libcec/decoder.c |   19 +++++++++++++++++--
 2 files changed, 51 insertions(+), 7 deletions(-)

-- 
1.7.5.4

[cecd-devel] [PATCH 2/6] [cecd] check and handle libcec_decode_message return values

[Florian F. <f.f...@gm...>]

In case we do not recognize the opcode, or the parameters supplied
are invalid it is mandatory to reply with a <Feature Abort> message
with the proper [Abort Reason].
---
 cecd/cecd.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index 48e62b0..acc9859 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -891,7 +891,10 @@ int main(int argc, char** argv)
 			cecd_log("could not read message (error %d)\n", len);
 			continue;
 		}
-		libcec_decode_message(buffer, len);
+		r = libcec_decode_message(buffer, len);
+		if (r != LIBCEC_SUCCESS)
+			buffer[1] = CEC_OP_ABORT;
+
 		if (len <= 1) {
 			// Ignore ACK, etc.
 			continue;
@@ -967,7 +970,17 @@ int main(int argc, char** argv)
 			if ((buffer[0] & 0x0f) == 0x0f)
 				break;
 			buffer[1] = CEC_OP_FEATURE_ABORT;
-			buffer[2] = CEC_ABORT_REFUSED;
+			switch (r) {
+			case LIBCEC_ERROR_NOT_SUPPORTED:
+				buffer[2] = CEC_ABORT_UNRECOGNIZED;
+				break;
+			case LIBCEC_ERROR_INVALID_PARAM:
+				buffer[2] = CEC_ABORT_INVALID_OPERAND;
+				break;
+			default:
+				buffer[2] = CEC_ABORT_REFUSED;
+				break;
+			}
 			len = 3;
 			break;
 
@@ -982,6 +995,7 @@ int main(int argc, char** argv)
 			len = 0;
 			break;
 		}
+
 		if (len) {
 			if (libcec_write_message(handle, buffer, len)) {
 				cecd_log("could not send message\n");
-- 
1.7.5.4

[cecd-devel] [PATCH 4/6] [libcec] check unicast/broadcast flag with destination address

[Florian F. <f.f...@gm...>]

HDMI CEC Compliance Testing Specification imposes a device not to answer to:

- messages received as broadcast while they are supposed to be directed
- messages received as directed with a broadcast address

in such a case return LIBCEC_ERROR_OTHER
---
 libcec/decoder.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/libcec/decoder.c b/libcec/decoder.c
index 37764c2..2181310 100644
--- a/libcec/decoder.c
+++ b/libcec/decoder.c
@@ -210,6 +210,17 @@ int libcec_decode_message(uint8_t* message, size_t length)
 		return LIBCEC_ERROR_NOT_SUPPORTED;
 	}
 
+	// Broadcasted messages received as directed messages
+	if ((dst == 0x0F) && ((msg_props[message[1]] & 0x40) == 0)) {
+		ceci_warn("broadcast message received as directed: %02X", message[1]);
+		return LIBCEC_ERROR_OTHER;
+	}
+
+	if ((dst != 0x0F) && ((msg_props[message[1]] & 0x20) == 0)) {
+		ceci_warn("directed message received as broadcast: %02X", message[1]);
+		return LIBCEC_ERROR_OTHER;
+	}
+
 	if ( (length-2 < msg_min_max[msg_props[message[1]]&0x1F][0])
 	  || (length-2 > msg_min_max[msg_props[message[1]]&0x1F][1]) ) {
 		  ceci_warn("invalid payload length for opcode: %02X", message[1]);
-- 
1.7.5.4

[cecd-devel] [PATCH 3/6] [libcec] compute src and dst once and for all

[Florian F. <f.f...@gm...>]

Once we made sure the message length is valid, compute src and dst address
once and do not do that twice.
---
 libcec/decoder.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libcec/decoder.c b/libcec/decoder.c
index 8fd7655..37764c2 100644
--- a/libcec/decoder.c
+++ b/libcec/decoder.c
@@ -196,9 +196,12 @@ int libcec_decode_message(uint8_t* message, size_t length)
 		return LIBCEC_ERROR_INVALID_PARAM;
 	}
 
+	src = message[0] >> 4;
+	dst = message[0] & 0x0F;
+
 	// Polling Message
 	if (length == 1) {
-		ceci_info("  o %1X->%1X: <Polling Message>", src = message[0] >> 4, dst = message[0] & 0x0F);
+		ceci_info("  o %1X->%1X: <Polling Message>", src, dst);
 		return LIBCEC_SUCCESS;
 	}
 
@@ -206,12 +209,13 @@ int libcec_decode_message(uint8_t* message, size_t length)
 		ceci_warn("unsupported Opcode: %02X", message[1]);
 		return LIBCEC_ERROR_NOT_SUPPORTED;
 	}
+
 	if ( (length-2 < msg_min_max[msg_props[message[1]]&0x1F][0])
 	  || (length-2 > msg_min_max[msg_props[message[1]]&0x1F][1]) ) {
 		  ceci_warn("invalid payload length for opcode: %02X", message[1]);
 		  return LIBCEC_ERROR_INVALID_PARAM;
 	}
-	ceci_info("  o %1X->%1X: <%s>", src = message[0] >> 4, dst = message[0] & 0x0F,
+	ceci_info("  o %1X->%1X: <%s>", src, dst,
 			  msg_description[msg_index[message[1]]]);
 	display_buffer_hex(message+1, length-1);
 
-- 
1.7.5.4

[cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Florian F. <f.f...@gm...>]

Per specification it is mandatory to answer to an <Abort> message with
a <Feature Abort> reply in case the initiator address is not broadcast.
Any [Abort Reason] can be used.
---
 cecd/cecd.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index 67c0a6c..48e62b0 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -962,6 +962,15 @@ int main(int argc, char** argv)
 			}
 			len = 0;
 			break;
+		case CEC_OP_ABORT:
+			// Only answer to abort if initiator address is not broadcast
+			if ((buffer[0] & 0x0f) == 0x0f)
+				break;
+			buffer[1] = CEC_OP_FEATURE_ABORT;
+			buffer[2] = CEC_ABORT_REFUSED;
+			len = 3;
+			break;
+
 		default:
 			// Convert to hash, to match against a conf file command
 			cec_unprocessed[cec_unprocessed_len++] = htab_hash(buffer+1, len-1, htab_cec, 0);
-- 
1.7.5.4

[cecd-devel] [PATCH 2/6 v2] [cecd] check and handle libcec_decode_message return values

[Florian F. <f.f...@gm...>]

In case we do not recognize the opcode, it is mandatory to reply with
a <Feature Abort> message with the proper [Abort Reason]. If the error
returned is INVALID_PARAM, which corresponds to an invalid payload length
or number of operands, do not reply per specification.
---
Changes since v1:
- added check to verify length is big enough to access opcode byte
- removed reply in case of invalid payload lenght as said by specification
- store opcode for insertion in CEC_OP_FEATURE_ABORT reply

 cecd/cecd.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index 48e62b0..f96347b 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -603,7 +603,7 @@ int main(int argc, char** argv)
 	uint16_t physical_address = 0xFFFF;
 	// TODO: check for seq_data overflow
 	uint16_t seq_data[CEC_MAX_COMMAND_SIZE], seq_len, ucp_unprocessed[CEC_MAX_COMMAND_SIZE], cec_unprocessed[CEC_MAX_COMMAND_SIZE];
-	uint8_t i, byte, buffer[CEC_MAX_COMMAND_SIZE];
+	uint8_t i, byte, buffer[CEC_MAX_COMMAND_SIZE], opcode = 0;
 	uint8_t ucp_unprocessed_len = 0, ucp_processed_len, cec_unprocessed_len = 0, cec_processed_len;
 	char *target_device, *device_name, *str = NULL, *saveptr = NULL, **key, *val;
 
@@ -891,7 +891,12 @@ int main(int argc, char** argv)
 			cecd_log("could not read message (error %d)\n", len);
 			continue;
 		}
-		libcec_decode_message(buffer, len);
+		r = libcec_decode_message(buffer, len);
+		if (r != LIBCEC_SUCCESS && len >= 2) {
+			opcode = buffer[1];
+			buffer[1] = CEC_OP_ABORT;
+		}
+
 		if (len <= 1) {
 			// Ignore ACK, etc.
 			continue;
@@ -967,8 +972,19 @@ int main(int argc, char** argv)
 			if ((buffer[0] & 0x0f) == 0x0f)
 				break;
 			buffer[1] = CEC_OP_FEATURE_ABORT;
-			buffer[2] = CEC_ABORT_REFUSED;
-			len = 3;
+			buffer[2] = opcode;
+			switch (r) {
+			case LIBCEC_ERROR_NOT_SUPPORTED:
+				buffer[3] = CEC_ABORT_UNRECOGNIZED;
+				break;
+			case LIBCEC_ERROR_INVALID_PARAM:
+				len = 0;
+				break;
+			default:
+				buffer[3] = CEC_ABORT_REFUSED;
+				break;
+			}
+			len = 4;
 			break;
 
 		default:
@@ -982,6 +998,7 @@ int main(int argc, char** argv)
 			len = 0;
 			break;
 		}
+
 		if (len) {
 			if (libcec_write_message(handle, buffer, len)) {
 				cecd_log("could not send message\n");
-- 
1.7.5.4

[cecd-devel] [PATCH 5/6] [cecd] handle LIBCEC_ERROR_OTHER from libcec_decode_message

[Florian F. <f.f...@gm...>]

In such a case do not answer anything as required by the specification
since the message header is invalid.
---
 cecd/cecd.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index acc9859..5e3e110 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -973,15 +973,21 @@ int main(int argc, char** argv)
 			switch (r) {
 			case LIBCEC_ERROR_NOT_SUPPORTED:
 				buffer[2] = CEC_ABORT_UNRECOGNIZED;
+				len = 3;
 				break;
 			case LIBCEC_ERROR_INVALID_PARAM:
 				buffer[2] = CEC_ABORT_INVALID_OPERAND;
+				len = 3;
+				break;
+			case LIBCEC_ERROR_OTHER:
+				// Should not answer anything to invalid headers
+				len = 0;
 				break;
 			default:
 				buffer[2] = CEC_ABORT_REFUSED;
+				len = 3;
 				break;
 			}
-			len = 3;
 			break;
 
 		default:
-- 
1.7.5.4

[cecd-devel] [PATCH 5/6 v2] [cecd] handle LIBCEC_ERROR_OTHER from libcec_decode_message

[Florian F. <f.f...@gm...>]

In such a case do not answer anything as required by the specification
since the message header is invalid.
---
Changes since v1:
- updated according to patch 2

 cecd/cecd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index f96347b..17c81eb 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -978,6 +978,7 @@ int main(int argc, char** argv)
 				buffer[3] = CEC_ABORT_UNRECOGNIZED;
 				break;
 			case LIBCEC_ERROR_INVALID_PARAM:
+			case LIBCEC_ERROR_OTHER:
 				len = 0;
 				break;
 			default:
-- 
1.7.5.4

[cecd-devel] [PATCH 6/6] [cecd] fix BROADCAST usage

[Florian F. <f.f...@gm...>]

In case the the device type does not match the first available corresponding
logical address, we would be forcing the initiator address to the device_type
instead of the current logical_address.
---
 cecd/cecd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/cecd/cecd.c b/cecd/cecd.c
index 5e3e110..331edd9 100644
--- a/cecd/cecd.c
+++ b/cecd/cecd.c
@@ -39,7 +39,7 @@
 #include "profile.h"
 #include "profile_helpers.h"
 
-#define BROADCAST (device_type<<4 | 0x0F)
+#define BROADCAST (0x0F)
 #define MIN(X,Y) ((X) < (Y) ? (X) : (Y))
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 
@@ -911,7 +911,7 @@ int main(int argc, char** argv)
 			len = i+2;
 			break;
 		case CEC_OP_GIVE_DEVICE_VENDOR_ID:
-			buffer[0] = BROADCAST;
+			buffer[0] |= BROADCAST;
 			buffer[1] = CEC_OP_DEVICE_VENDOR_ID;
 			buffer[2] = (device_oui>>16)&0xFF;
 			buffer[3] = (device_oui>>8)&0xFF;
@@ -934,7 +934,7 @@ int main(int argc, char** argv)
 			len = 3;
 			break;
 		case CEC_OP_GIVE_PHYSICAL_ADDRESS:
-			buffer[0] = BROADCAST;
+			buffer[0] |= BROADCAST;
 			buffer[1] = CEC_OP_REPORT_PHYSICAL_ADDRESS;
 			buffer[2] = physical_address >> 8;
 			buffer[3] = physical_address & 0xFF;
@@ -945,7 +945,7 @@ int main(int argc, char** argv)
 			// Ignore if request is for a different phys_addr
 			if ((buffer[2] != (physical_address >> 8)) || (buffer[3] != (physical_address & 0xFF)))
 				break;
-			buffer[0] = BROADCAST;
+			buffer[0] |= BROADCAST;
 			buffer[1] = CEC_OP_ACTIVE_SOURCE;
 			buffer[2] = physical_address >> 8;
 			buffer[3] = physical_address & 0xFF;
-- 
1.7.5.4

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Florian F. <f.f...@gm...>]

Hello Pete,

On 12/30/11 15:10, Florian Fainelli wrote:
> Per specification it is mandatory to answer to an<Abort>  message with
> a<Feature Abort>  reply in case the initiator address is not broadcast.
> Any [Abort Reason] can be used.

First of all, let me wish you a happy new year. This patch is missing 
the opcode parameter in the <Feature Abort> message, will fix that.

> ---
>   cecd/cecd.c |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/cecd/cecd.c b/cecd/cecd.c
> index 67c0a6c..48e62b0 100644
> --- a/cecd/cecd.c
> +++ b/cecd/cecd.c
> @@ -962,6 +962,15 @@ int main(int argc, char** argv)
>   			}
>   			len = 0;
>   			break;
> +		case CEC_OP_ABORT:
> +			// Only answer to abort if initiator address is not broadcast
> +			if ((buffer[0]&  0x0f) == 0x0f)
> +				break;
> +			buffer[1] = CEC_OP_FEATURE_ABORT;
> +			buffer[2] = CEC_ABORT_REFUSED;
> +			len = 3;
> +			break;
> +
>   		default:
>   			// Convert to hash, to match against a conf file command
>   			cec_unprocessed[cec_unprocessed_len++] = htab_hash(buffer+1, len-1, htab_cec, 0);

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Pete B. <pb...@gm...>]

Hi Florian, and happy new year to you too!

Thanks for the patches. I'll try to push 3-6 tonight or tomorrow, after 
I have reviewed them. I'll wait for your update on #1 (and possibly #2 
if related?)

Regards,

/Pete

On 2012.01.02 12:49, Florian Fainelli wrote:
> Hello Pete,
>
> On 12/30/11 15:10, Florian Fainelli wrote:
>> Per specification it is mandatory to answer to an<Abort> message with
>> a<Feature Abort> reply in case the initiator address is not broadcast.
>> Any [Abort Reason] can be used.
>
> First of all, let me wish you a happy new year. This patch is missing
> the opcode parameter in the <Feature Abort> message, will fix that.
>
>> ---
>> cecd/cecd.c | 9 +++++++++
>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/cecd/cecd.c b/cecd/cecd.c
>> index 67c0a6c..48e62b0 100644
>> --- a/cecd/cecd.c
>> +++ b/cecd/cecd.c
>> @@ -962,6 +962,15 @@ int main(int argc, char** argv)
>> }
>> len = 0;
>> break;
>> + case CEC_OP_ABORT:
>> + // Only answer to abort if initiator address is not broadcast
>> + if ((buffer[0]& 0x0f) == 0x0f)
>> + break;
>> + buffer[1] = CEC_OP_FEATURE_ABORT;
>> + buffer[2] = CEC_ABORT_REFUSED;
>> + len = 3;
>> + break;
>> +
>> default:
>> // Convert to hash, to match against a conf file command
>> cec_unprocessed[cec_unprocessed_len++] = htab_hash(buffer+1, len-1,
>> htab_cec, 0);

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Florian F. <f.f...@gm...>]

Hello Pete,

On 01/02/12 18:27, Pete Batard wrote:
> Hi Florian, and happy new year to you too!
>
> Thanks for the patches. I'll try to push 3-6 tonight or tomorrow, after
> I have reviewed them. I'll wait for your update on #1 (and possibly #2
> if related?)

I just send an updated patch #2 and #5 which were both affected by the 
change. The rest of the patches should apply fine if I am not mistaking.

Thank you.
--
Florian

>
> Regards,
>
> /Pete
>
> On 2012.01.02 12:49, Florian Fainelli wrote:
>> Hello Pete,
>>
>> On 12/30/11 15:10, Florian Fainelli wrote:
>>> Per specification it is mandatory to answer to an<Abort>  message with
>>> a<Feature Abort>  reply in case the initiator address is not broadcast.
>>> Any [Abort Reason] can be used.
>>
>> First of all, let me wish you a happy new year. This patch is missing
>> the opcode parameter in the<Feature Abort>  message, will fix that.
>>
>>> ---
>>> cecd/cecd.c | 9 +++++++++
>>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cecd/cecd.c b/cecd/cecd.c
>>> index 67c0a6c..48e62b0 100644
>>> --- a/cecd/cecd.c
>>> +++ b/cecd/cecd.c
>>> @@ -962,6 +962,15 @@ int main(int argc, char** argv)
>>> }
>>> len = 0;
>>> break;
>>> + case CEC_OP_ABORT:
>>> + // Only answer to abort if initiator address is not broadcast
>>> + if ((buffer[0]&  0x0f) == 0x0f)
>>> + break;
>>> + buffer[1] = CEC_OP_FEATURE_ABORT;
>>> + buffer[2] = CEC_ABORT_REFUSED;
>>> + len = 3;
>>> + break;
>>> +
>>> default:
>>> // Convert to hash, to match against a conf file command
>>> cec_unprocessed[cec_unprocessed_len++] = htab_hash(buffer+1, len-1,
>>> htab_cec, 0);
>
>
> ------------------------------------------------------------------------------
> Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don't need a complex
> infrastructure or vast IT resources to deliver seamless, secure access to
> virtual desktops. With this all-in-one solution, easily deploy virtual
> desktops for less than the cost of PCs and save 60% on VDI infrastructure
> costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox
> _______________________________________________
> cecd-devel mailing list
> cec...@li...
> https://lists.sourceforge.net/lists/listinfo/cecd-devel

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Pete B. <pb...@gm...>]

On 2012.01.03 13:10, Florian Fainelli wrote:
> I just send an updated patch #2 and #5 which were both affected by the
> change. The rest of the patches should apply fine if I am not mistaking.

I have now applied and pushed #1 to #5 (with a slightly modified #2 as 
we can remove the len >= 2 check).

I'll apply #6 later on today, mostly because I'm trying to remember why 
I was doing it the way I did.

Thanks again!

/Pete

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Florian F. <f.f...@gm...>]

On 01/03/12 16:39, Pete Batard wrote:
> On 2012.01.03 13:10, Florian Fainelli wrote:
>> I just send an updated patch #2 and #5 which were both affected by the
>> change. The rest of the patches should apply fine if I am not mistaking.
>
> I have now applied and pushed #1 to #5 (with a slightly modified #2 as
> we can remove the len>= 2 check).
>
> I'll apply #6 later on today, mostly because I'm trying to remember why
> I was doing it the way I did.

Allright, I think this needs some clarification anyway.

>
> Thanks again!

You are welcome.

>
> /Pete
>
> ------------------------------------------------------------------------------
> Write once. Port to many.
> Get the SDK and tools to simplify cross-platform app development. Create
> new or port existing apps to sell to consumers worldwide. Explore the
> Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
> http://p.sf.net/sfu/intel-appdev
> _______________________________________________
> cecd-devel mailing list
> cec...@li...
> https://lists.sourceforge.net/lists/listinfo/cecd-devel

Re: [cecd-devel] [PATCH 1/6] [cecd] answer to <Abort> opcode

[Pete B. <pb...@gm...>]

On 2012.01.03 15:50, Florian Fainelli wrote:
>> I'll apply #6 later on today, mostly because I'm trying to remember why
>> I was doing it the way I did.
>
> Allright, I think this needs some clarification anyway.

Sorry for the delay. I have now pushed a replacement fix for #6 as I 
seem to remember that the reason device_type was used is because I 
didn't have logical address allocation when I coded it, so I used 
device_type as an easy stopgap. Now that logical address allocation is 
done, the logical address should be the one used.

I also pushed a version bump, since it's been a long time, but that is 
pretty much only used to add an internal version tag for quick reference 
- not sure it is actually useful on this project.

Regards,

/Pete