Sensor status message. Property length (NCS 1.6)

[ipshiv]

Good day!

I have a question about length in MPID. In current implementation size measures in uint16 size and this brings lot's of troubles when you try to decode values: 8 bit and 16 bit have same size, 24 bit and 32 bit does also.

Is it a bug and size should be calculated in bytes or I missed something in Mesh model specification?

[Elfving]

Hey Ivan!

Just letting you know that I am looking into it, but I will have to get back to you.

Best regards,

Elfving

[Elfving]

Hey again Ivan! Sorry about the delay.

The length field should give a number of bytes, or octets. Where in our code are you seeing this? 

Best regards,

Elfving

[ipshiv]

Sorry for late response.

We had a sensor server \ client, more than a year ago, written for MESH SDK and it still working on legacy devices (new one use NCS).

We used next function to parse incoming data

typedef struct __attribute((packed))
{
	uint8_t a_b_format : 1;					/**< Format A tag, 0b0. The Format field is a 1-bit bit field that identifies the format of the Length and Property ID fields, as decribed 4.2.14 Sensor Status */
	uint8_t length : 4;					/**< Length of the Property Value */
	uint16_t prop_id : 11;					/**< Property identifying a sensor */
} generic_sensor_format_a_header_t;


static void parse_status_data(uint16_t src, uint16_t dst,
				const uint8_t *p_buff, uint16_t length)
{
	uint16_t shift = 0;
	generic_sensor_format_a_header_t *header;
	uint8_t i = 0;
	while (shift < length) {
		header = (generic_sensor_format_a_header_t *)(p_buff + shift);

		shift += (sizeof(generic_sensor_format_a_header_t));
		i++;
		__LOG(LOG_SRC_APP,
		      LOG_LEVEL_DBG3,
		      "%d >> ID:%d Length:%d, left %d\n",
		      i,
		      header->prop_id,
		      header->length,
		      length - header->length);
 		size_t data_size = header->length * sizeof(uint16_t);
		shift += data_size;
	}
}

And, for example, I receive data from NCS's sensor server with data

1 >> ID:82 Length:2, left 23

82 in decimal equals 0x0052

#define BT_MESH_PROP_ID_PRESENT_DEV_INPUT_POWER 0x0052
/** Present device input power
 *
 *  Channels:
 *  - Present Device Input Power
 *    - Unit: Watt
 *    - Encoding: 24 bit unsigned scalar (Resolution: 0.1 W)
 *    - Range: 0 to 1677721.4
 */
extern const struct bt_mesh_sensor_type bt_mesh_sensor_present_dev_input_power;

Of course it ruins all marshalled data

[ipshiv]

I think I've found where it comes from. Check out this line:

https://github.com/nrfconnect/sdk-nrf/blob/7f9018b11836b3c4d2049fce626a8ffa9770ad4c/subsys/bluetooth/mesh/sensor.c#L84

int sensor_status_id_encode(struct net_buf_simple *buf, uint8_t len, uint16_t id)
{
	if ((len > 0 && len <= 16) && id < 2048) {
		if (net_buf_simple_tailroom(buf) < 2 + len) {
			return -ENOMEM;
		}

		net_buf_simple_add_le16(buf, ((len - 1) << 1) | (id << 5));
	} else {
		if (net_buf_simple_tailroom(buf) < 3 + len) {
			return -ENOMEM;
		}

		net_buf_simple_add_u8(buf, BIT(0) | (((len - 1) & BIT_MASK(7))
						     << 1));
		net_buf_simple_add_le16(buf, id);
	}

	return 0;
}

To header goes (len - 1)

[Elfving]

Hey Ivan,

It seems to me that our implementation here is correct. However there seems to be somewhat of an off-by-one mistake in MshMDLv1.0.1, section 4.2.14. It should say that "Property values longer than 127 octets are not supported by the Sensor Status message." It will be fixed in newer versions.

When length <=16 and Property ID < 2048: The Length field is a 1-based uint4 value (valid range 0x0–0xF, representing range of 1–16).
When length <=127 and Property ID < 65535: The Length field is a 1-based uint7 value (valid range 0x0–0x7F, representing range of 1–127). The value 0x7F represents a length of zero.

Best regards,

Elfving