2018-03-22 10:44:42

by Robert Lubaś

[permalink] [raw]
Subject: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

In Mesh Profile spec 4.2.17.4 Heartbeat Publication TTL value range is
0x00-0x7F. In cmd_hb_pub_set heartbeat ttl was set to DEFAULT_TTL 0xFF, this
patch fix this by adding ttl param to hb-pub-set.
---
mesh/config-client.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mesh/config-client.c b/mesh/config-client.c
index 19e617d62..0b5b8677b 100644
--- a/mesh/config-client.c
+++ b/mesh/config-client.c
@@ -1042,7 +1042,7 @@ static void cmd_hb_pub_set(int argc, char *argv[])
n = mesh_opcode_set(OP_CONFIG_HEARTBEAT_PUB_SET, msg);

parm_cnt = read_input_parameters(argc, argv);
- if (parm_cnt != 5) {
+ if (parm_cnt != 6) {
bt_shell_printf("Bad arguments: %s\n", argv[1]);
return bt_shell_noninteractive_quit(EXIT_FAILURE);
}
@@ -1056,12 +1056,12 @@ static void cmd_hb_pub_set(int argc, char *argv[])
/* Period Log */
msg[n++] = parms[2];
/* Heartbeat TTL */
- msg[n++] = DEFAULT_TTL;
+ msg[n++] = parms[3];
/* Features */
- put_le16(parms[3], msg + n);
+ put_le16(parms[4], msg + n);
n += 2;
/* NetKey Index */
- put_le16(parms[4], msg + n);
+ put_le16(parms[5], msg + n);
n += 2;

if (!config_send(msg, n)) {
@@ -1167,8 +1167,8 @@ static const struct bt_shell_menu cfg_menu = {
"Set relay"},
{"relay-get", NULL, cmd_relay_get,
"Get relay"},
- {"hb-pub-set", "<pub_addr> <count> <period> <features> <net_idx>",
- cmd_hb_pub_set, "Set heartbeat publish"},
+ {"hb-pub-set", "<pub_addr> <count> <period> <ttl> <features> <net_idx>",
+ cmd_hb_pub_set, "Set heartbeat publish"},
{"hb-pub-get", NULL, cmd_hb_pub_get,
"Get heartbeat publish"},
{"hb-sub-set", "<src_addr> <dst_addr> <period>",
--
2.11.0



2018-03-28 12:48:08

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

Hi Robert,

On Wed, Mar 28, 2018, Robert Lubas wrote:
> The problem is with the TTL field in the message (not valid value is hard
> codded). I proposed a fix by passing ttl into cmd_hb_pub_set (user has to be
> aware of valid values of the TTL as well as of another parameters in this
> function)
>
> In the 4.4.1.2.15 Heartbeat Publication state (Mesh Profile spec) in the 3rd
> paragraph we can read that if the message is successfully processed (no
> invalid key index error) we should update the Heartbeat Publication state to
> the corresponding field values (defined in Table 4.120). Going to this table
> the Heartbeat Publication TTL state should be updated using message field
> TTL. The message field TTL has to be set according to the 4.2.17.4 Heartbeat
> Publication TTL (0x00-0x7F values are valid).
>
> In the spec there is nothing about translation to the system settings
> mechanism in case of heartbeat.
>
> Kindly asking for the review.

I think Brian is on vacation, so there will likely be no response from
him for awhile. The patch looks good to me, and even though we have the
default TTL concept, for testing purposes it's nice to be able to set
the TTL explicitly for the heartbeat configuration.

So, the patch has now been applied upstream. Thanks.

Johan

2018-03-28 11:43:51

by Robert Lubaś

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

Hi

On 03/23/2018 05:08 PM, Robert Lubas wrote:
> Hi Brian,
>
> On 03/23/2018 01:37 PM, Gix, Brian wrote:
>> Hi Robert,
>>
>>> On Mar 23, 2018, at 8:20 AM, Robert Lubas <[email protected]>
>>> wrote:
>>>
>>> Hi Brian,
>>>
>>>> On 03/22/2018 10:33 PM, Gix, Brian wrote:
>>>> I am unable to look at all the code in context at the moment, but
>>>> using the DEFAULT_TTL (0xff) for any TTL value, will automatically
>>>> get replaced with whatever the default TTL has been set to. It is a
>>>> handy value which means “use the system setting” without needing to
>>>> look it up.
>>>>> On Mar 22, 2018, at 3:45 AM, Robert Lubaś
>>>>> <[email protected]> wrote:
>>>>>
>>>>> In Mesh Profile spec 4.2.17.4 Heartbeat Publication TTL value range is
>>>>> 0x00-0x7F. In cmd_hb_pub_set heartbeat ttl was set to DEFAULT_TTL
>>>>> 0xFF, this
>>>>> patch fix this by adding ttl param to hb-pub-set.
>>>>> ---
>>>>> mesh/config-client.c | 12 ++++++------
>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mesh/config-client.c b/mesh/config-client.c
>>>>> index 19e617d62..0b5b8677b 100644
>>>>> --- a/mesh/config-client.c
>>>>> +++ b/mesh/config-client.c
>>>>> @@ -1042,7 +1042,7 @@ static void cmd_hb_pub_set(int argc, char
>>>>> *argv[])
>>>>>     n = mesh_opcode_set(OP_CONFIG_HEARTBEAT_PUB_SET, msg);
>>>>>
>>>>>     parm_cnt = read_input_parameters(argc, argv);
>>>>> -    if (parm_cnt != 5) {
>>>>> +    if (parm_cnt != 6) {
>>>>>         bt_shell_printf("Bad arguments: %s\n", argv[1]);
>>>>>         return bt_shell_noninteractive_quit(EXIT_FAILURE);
>>>>>     }
>>>>> @@ -1056,12 +1056,12 @@ static void cmd_hb_pub_set(int argc, char
>>>>> *argv[])
>>>>>     /* Period Log */
>>>>>     msg[n++] = parms[2];
>>>>>     /* Heartbeat TTL */
>>>>> -    msg[n++] = DEFAULT_TTL;
>>>>> +    msg[n++] = parms[3];
>>>>>     /* Features */
>>>>> -    put_le16(parms[3], msg + n);
>>>>> +    put_le16(parms[4], msg + n);
>>>>>     n += 2;
>>>>>     /* NetKey Index */
>>>>> -    put_le16(parms[4], msg + n);
>>>>> +    put_le16(parms[5], msg + n);
>>>>>     n += 2;
>>>>>
>>>>>     if (!config_send(msg, n)) {
>>>>> @@ -1167,8 +1167,8 @@ static const struct bt_shell_menu cfg_menu = {
>>>>>                         "Set relay"},
>>>>>     {"relay-get",           NULL,                   cmd_relay_get,
>>>>>                         "Get relay"},
>>>>> -    {"hb-pub-set", "<pub_addr> <count> <period> <features>
>>>>> <net_idx>",
>>>>> -                cmd_hb_pub_set,     "Set heartbeat publish"},
>>>>> +    {"hb-pub-set", "<pub_addr> <count> <period> <ttl> <features>
>>>>> <net_idx>",
>>>>> +                cmd_hb_pub_set,    "Set heartbeat publish"},
>>>>>     {"hb-pub-get",           NULL,                   cmd_hb_pub_get,
>>>>>                         "Get heartbeat publish"},
>>>>>     {"hb-sub-set", "<src_addr> <dst_addr> <period>",
>>>>> --
>>>>> 2.11.0
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-bluetooth" in
>>>>> the body of a message to [email protected]
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> I see your point. You know the mesh specification very well, but I
>>> will just refer to it to construct the reply. The Mesh Profile
>>> specification says: if the publish TTL is set to 0xFF the message use
>>> the Default TTL (4.2.2.5). The Default TTL valid values are 0x00,
>>> 0x02-0x7F. Prohibited are 0x01, 0x80-0x7FF (4.2.7).
>>>
>>> However in terms of Heartbeat publication TTL the valid values are
>>> 0x00-0x7F. Prohibited are 0x80-0xFF (4.2.17.4). Taking into account
>>> valid ranges the heartbeat TTL should be masked with 0x7F or in case
>>> of 0xFF the node Default TTL from db should be used.
>>>
>>> I think that it will be helpful e.g. to assess the basic reliability
>>> of the mesh network having the heartbeat publication TTL as a
>>> parameter. Then, for example you can very easy build the metric which
>>> depends on TTL.
>>
>> You are correct that you should never see anything greater than 0x7f
>> over-the-air.... but the intent was that the lower layers would handle
>> the translation to the system setting.... or in the case of heartbeat,
>> the control layer.
>>
> Can you expand a little bit more about handling the translation to the
> system setting?
>
> The Default TTL is applied to access layer, the heartbeat operates on
> upper transport layer - so from Configuration Client point of view all
> above 0x7F are just prohibited.The current implementation of heartbeat publication set does not work.
The problem is with the TTL field in the message (not valid value is
hard codded). I proposed a fix by passing ttl into cmd_hb_pub_set (user
has to be aware of valid values of the TTL as well as of another
parameters in this function)

In the 4.4.1.2.15 Heartbeat Publication state (Mesh Profile spec) in the
3rd paragraph we can read that if the message is successfully processed
(no invalid key index error) we should update the Heartbeat Publication
state to the corresponding field values (defined in Table 4.120). Going
to this table the Heartbeat Publication TTL state should be updated
using message field TTL. The message field TTL has to be set according
to the 4.2.17.4 Heartbeat Publication TTL (0x00-0x7F values are valid).

In the spec there is nothing about translation to the system settings
mechanism in case of heartbeat.

Kindly asking for the review.

2018-03-23 16:08:40

by Robert Lubaś

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

Hi Brian,

On 03/23/2018 01:37 PM, Gix, Brian wrote:
> Hi Robert,
>
>> On Mar 23, 2018, at 8:20 AM, Robert Lubas <[email protected]> wrote:
>>
>> Hi Brian,
>>
>>> On 03/22/2018 10:33 PM, Gix, Brian wrote:
>>> I am unable to look at all the code in context at the moment, but using the DEFAULT_TTL (0xff) for any TTL value, will automatically get replaced with whatever the default TTL has been set to. It is a handy value which means “use the system setting” without needing to look it up.
>>>> On Mar 22, 2018, at 3:45 AM, Robert Lubaś <[email protected]> wrote:
>>>>
>>>> In Mesh Profile spec 4.2.17.4 Heartbeat Publication TTL value range is
>>>> 0x00-0x7F. In cmd_hb_pub_set heartbeat ttl was set to DEFAULT_TTL 0xFF, this
>>>> patch fix this by adding ttl param to hb-pub-set.
>>>> ---
>>>> mesh/config-client.c | 12 ++++++------
>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/mesh/config-client.c b/mesh/config-client.c
>>>> index 19e617d62..0b5b8677b 100644
>>>> --- a/mesh/config-client.c
>>>> +++ b/mesh/config-client.c
>>>> @@ -1042,7 +1042,7 @@ static void cmd_hb_pub_set(int argc, char *argv[])
>>>> n = mesh_opcode_set(OP_CONFIG_HEARTBEAT_PUB_SET, msg);
>>>>
>>>> parm_cnt = read_input_parameters(argc, argv);
>>>> - if (parm_cnt != 5) {
>>>> + if (parm_cnt != 6) {
>>>> bt_shell_printf("Bad arguments: %s\n", argv[1]);
>>>> return bt_shell_noninteractive_quit(EXIT_FAILURE);
>>>> }
>>>> @@ -1056,12 +1056,12 @@ static void cmd_hb_pub_set(int argc, char *argv[])
>>>> /* Period Log */
>>>> msg[n++] = parms[2];
>>>> /* Heartbeat TTL */
>>>> - msg[n++] = DEFAULT_TTL;
>>>> + msg[n++] = parms[3];
>>>> /* Features */
>>>> - put_le16(parms[3], msg + n);
>>>> + put_le16(parms[4], msg + n);
>>>> n += 2;
>>>> /* NetKey Index */
>>>> - put_le16(parms[4], msg + n);
>>>> + put_le16(parms[5], msg + n);
>>>> n += 2;
>>>>
>>>> if (!config_send(msg, n)) {
>>>> @@ -1167,8 +1167,8 @@ static const struct bt_shell_menu cfg_menu = {
>>>> "Set relay"},
>>>> {"relay-get", NULL, cmd_relay_get,
>>>> "Get relay"},
>>>> - {"hb-pub-set", "<pub_addr> <count> <period> <features> <net_idx>",
>>>> - cmd_hb_pub_set, "Set heartbeat publish"},
>>>> + {"hb-pub-set", "<pub_addr> <count> <period> <ttl> <features> <net_idx>",
>>>> + cmd_hb_pub_set, "Set heartbeat publish"},
>>>> {"hb-pub-get", NULL, cmd_hb_pub_get,
>>>> "Get heartbeat publish"},
>>>> {"hb-sub-set", "<src_addr> <dst_addr> <period>",
>>>> --
>>>> 2.11.0
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> I see your point. You know the mesh specification very well, but I will just refer to it to construct the reply. The Mesh Profile specification says: if the publish TTL is set to 0xFF the message use the Default TTL (4.2.2.5). The Default TTL valid values are 0x00, 0x02-0x7F. Prohibited are 0x01, 0x80-0x7FF (4.2.7).
>>
>> However in terms of Heartbeat publication TTL the valid values are 0x00-0x7F. Prohibited are 0x80-0xFF (4.2.17.4). Taking into account valid ranges the heartbeat TTL should be masked with 0x7F or in case of 0xFF the node Default TTL from db should be used.
>>
>> I think that it will be helpful e.g. to assess the basic reliability of the mesh network having the heartbeat publication TTL as a parameter. Then, for example you can very easy build the metric which depends on TTL.
>
> You are correct that you should never see anything greater than 0x7f over-the-air.... but the intent was that the lower layers would handle the translation to the system setting.... or in the case of heartbeat, the control layer.
>
Can you expand a little bit more about handling the translation to the
system setting?

The Default TTL is applied to access layer, the heartbeat operates on
upper transport layer - so from Configuration Client point of view all
above 0x7F are just prohibited.

2018-03-23 12:37:17

by Gix, Brian

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

SGkgUm9iZXJ0LA0KDQo+IE9uIE1hciAyMywgMjAxOCwgYXQgODoyMCBBTSwgUm9iZXJ0IEx1YmFz
IDxyb2JlcnQubHViYXNAc2lsdmFpci5jb20+IHdyb3RlOg0KPiANCj4gSGkgQnJpYW4sDQo+IA0K
Pj4gT24gMDMvMjIvMjAxOCAxMDozMyBQTSwgR2l4LCBCcmlhbiB3cm90ZToNCj4+IEkgYW0gdW5h
YmxlIHRvIGxvb2sgYXQgYWxsIHRoZSBjb2RlIGluIGNvbnRleHQgYXQgdGhlIG1vbWVudCwgYnV0
IHVzaW5nIHRoZSBERUZBVUxUX1RUTCAoMHhmZikgZm9yIGFueSBUVEwgdmFsdWUsIHdpbGwgYXV0
b21hdGljYWxseSBnZXQgcmVwbGFjZWQgd2l0aCB3aGF0ZXZlciB0aGUgZGVmYXVsdCBUVEwgaGFz
IGJlZW4gc2V0IHRvLiBJdCBpcyBhIGhhbmR5IHZhbHVlIHdoaWNoIG1lYW5zIOKAnHVzZSB0aGUg
c3lzdGVtIHNldHRpbmfigJ0gd2l0aG91dCBuZWVkaW5nIHRvIGxvb2sgaXQgdXAuDQo+Pj4gT24g
TWFyIDIyLCAyMDE4LCBhdCAzOjQ1IEFNLCBSb2JlcnQgTHViYcWbIDxyb2JlcnQubHViYXNAc2ls
dmFpci5jb20+IHdyb3RlOg0KPj4+IA0KPj4+IEluIE1lc2ggUHJvZmlsZSBzcGVjIDQuMi4xNy40
IEhlYXJ0YmVhdCBQdWJsaWNhdGlvbiBUVEwgdmFsdWUgcmFuZ2UgaXMNCj4+PiAweDAwLTB4N0Yu
IEluIGNtZF9oYl9wdWJfc2V0IGhlYXJ0YmVhdCB0dGwgd2FzIHNldCB0byBERUZBVUxUX1RUTCAw
eEZGLCB0aGlzDQo+Pj4gcGF0Y2ggZml4IHRoaXMgYnkgYWRkaW5nIHR0bCBwYXJhbSB0byBoYi1w
dWItc2V0Lg0KPj4+IC0tLQ0KPj4+IG1lc2gvY29uZmlnLWNsaWVudC5jIHwgMTIgKysrKysrLS0t
LS0tDQo+Pj4gMSBmaWxlIGNoYW5nZWQsIDYgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkN
Cj4+PiANCj4+PiBkaWZmIC0tZ2l0IGEvbWVzaC9jb25maWctY2xpZW50LmMgYi9tZXNoL2NvbmZp
Zy1jbGllbnQuYw0KPj4+IGluZGV4IDE5ZTYxN2Q2Mi4uMGI1Yjg2NzdiIDEwMDY0NA0KPj4+IC0t
LSBhL21lc2gvY29uZmlnLWNsaWVudC5jDQo+Pj4gKysrIGIvbWVzaC9jb25maWctY2xpZW50LmMN
Cj4+PiBAQCAtMTA0Miw3ICsxMDQyLDcgQEAgc3RhdGljIHZvaWQgY21kX2hiX3B1Yl9zZXQoaW50
IGFyZ2MsIGNoYXIgKmFyZ3ZbXSkNCj4+PiAgICBuID0gbWVzaF9vcGNvZGVfc2V0KE9QX0NPTkZJ
R19IRUFSVEJFQVRfUFVCX1NFVCwgbXNnKTsNCj4+PiANCj4+PiAgICBwYXJtX2NudCA9IHJlYWRf
aW5wdXRfcGFyYW1ldGVycyhhcmdjLCBhcmd2KTsNCj4+PiAtICAgIGlmIChwYXJtX2NudCAhPSA1
KSB7DQo+Pj4gKyAgICBpZiAocGFybV9jbnQgIT0gNikgew0KPj4+ICAgICAgICBidF9zaGVsbF9w
cmludGYoIkJhZCBhcmd1bWVudHM6ICVzXG4iLCBhcmd2WzFdKTsNCj4+PiAgICAgICAgcmV0dXJu
IGJ0X3NoZWxsX25vbmludGVyYWN0aXZlX3F1aXQoRVhJVF9GQUlMVVJFKTsNCj4+PiAgICB9DQo+
Pj4gQEAgLTEwNTYsMTIgKzEwNTYsMTIgQEAgc3RhdGljIHZvaWQgY21kX2hiX3B1Yl9zZXQoaW50
IGFyZ2MsIGNoYXIgKmFyZ3ZbXSkNCj4+PiAgICAvKiBQZXJpb2QgTG9nICovDQo+Pj4gICAgbXNn
W24rK10gPSBwYXJtc1syXTsNCj4+PiAgICAvKiBIZWFydGJlYXQgVFRMICovDQo+Pj4gLSAgICBt
c2dbbisrXSA9IERFRkFVTFRfVFRMOw0KPj4+ICsgICAgbXNnW24rK10gPSBwYXJtc1szXTsNCj4+
PiAgICAvKiBGZWF0dXJlcyAqLw0KPj4+IC0gICAgcHV0X2xlMTYocGFybXNbM10sIG1zZyArIG4p
Ow0KPj4+ICsgICAgcHV0X2xlMTYocGFybXNbNF0sIG1zZyArIG4pOw0KPj4+ICAgIG4gKz0gMjsN
Cj4+PiAgICAvKiBOZXRLZXkgSW5kZXggKi8NCj4+PiAtICAgIHB1dF9sZTE2KHBhcm1zWzRdLCBt
c2cgKyBuKTsNCj4+PiArICAgIHB1dF9sZTE2KHBhcm1zWzVdLCBtc2cgKyBuKTsNCj4+PiAgICBu
ICs9IDI7DQo+Pj4gDQo+Pj4gICAgaWYgKCFjb25maWdfc2VuZChtc2csIG4pKSB7DQo+Pj4gQEAg
LTExNjcsOCArMTE2Nyw4IEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgYnRfc2hlbGxfbWVudSBjZmdf
bWVudSA9IHsNCj4+PiAgICAgICAgICAgICAgICAgICAgICAgICJTZXQgcmVsYXkifSwNCj4+PiAg
ICB7InJlbGF5LWdldCIsICAgICAgICAgICBOVUxMLCAgICAgICAgICAgICAgICAgICBjbWRfcmVs
YXlfZ2V0LA0KPj4+ICAgICAgICAgICAgICAgICAgICAgICAgIkdldCByZWxheSJ9LA0KPj4+IC0g
ICAgeyJoYi1wdWItc2V0IiwgIjxwdWJfYWRkcj4gPGNvdW50PiA8cGVyaW9kPiA8ZmVhdHVyZXM+
IDxuZXRfaWR4PiIsDQo+Pj4gLSAgICAgICAgICAgICAgICBjbWRfaGJfcHViX3NldCwgICAgICJT
ZXQgaGVhcnRiZWF0IHB1Ymxpc2gifSwNCj4+PiArICAgIHsiaGItcHViLXNldCIsICI8cHViX2Fk
ZHI+IDxjb3VudD4gPHBlcmlvZD4gPHR0bD4gPGZlYXR1cmVzPiA8bmV0X2lkeD4iLA0KPj4+ICsg
ICAgICAgICAgICAgICAgY21kX2hiX3B1Yl9zZXQsICAgICJTZXQgaGVhcnRiZWF0IHB1Ymxpc2gi
fSwNCj4+PiAgICB7ImhiLXB1Yi1nZXQiLCAgICAgICAgICAgTlVMTCwgICAgICAgICAgICAgICAg
ICAgY21kX2hiX3B1Yl9nZXQsDQo+Pj4gICAgICAgICAgICAgICAgICAgICAgICAiR2V0IGhlYXJ0
YmVhdCBwdWJsaXNoIn0sDQo+Pj4gICAgeyJoYi1zdWItc2V0IiwgIjxzcmNfYWRkcj4gPGRzdF9h
ZGRyPiA8cGVyaW9kPiIsDQo+Pj4gLS0gDQo+Pj4gMi4xMS4wDQo+Pj4gDQo+Pj4gLS0NCj4+PiBU
byB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUg
bGludXgtYmx1ZXRvb3RoIiBpbg0KPj4+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRv
bW9Admdlci5rZXJuZWwub3JnDQo+Pj4gTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3Zn
ZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQo+IEkgc2VlIHlvdXIgcG9pbnQuIFlv
dSBrbm93IHRoZSBtZXNoIHNwZWNpZmljYXRpb24gdmVyeSB3ZWxsLCBidXQgSSB3aWxsIGp1c3Qg
cmVmZXIgdG8gaXQgdG8gY29uc3RydWN0IHRoZSByZXBseS4gVGhlIE1lc2ggUHJvZmlsZSBzcGVj
aWZpY2F0aW9uIHNheXM6IGlmIHRoZSBwdWJsaXNoIFRUTCBpcyBzZXQgdG8gMHhGRiB0aGUgbWVz
c2FnZSB1c2UgdGhlIERlZmF1bHQgVFRMICg0LjIuMi41KS4gVGhlIERlZmF1bHQgVFRMIHZhbGlk
IHZhbHVlcyBhcmUgMHgwMCwgMHgwMi0weDdGLiBQcm9oaWJpdGVkIGFyZSAweDAxLCAweDgwLTB4
N0ZGICg0LjIuNykuDQo+IA0KPiBIb3dldmVyIGluIHRlcm1zIG9mIEhlYXJ0YmVhdCBwdWJsaWNh
dGlvbiBUVEwgdGhlIHZhbGlkIHZhbHVlcyBhcmUgMHgwMC0weDdGLiBQcm9oaWJpdGVkIGFyZSAw
eDgwLTB4RkYgKDQuMi4xNy40KS4gVGFraW5nIGludG8gYWNjb3VudCB2YWxpZCByYW5nZXMgdGhl
IGhlYXJ0YmVhdCBUVEwgc2hvdWxkIGJlIG1hc2tlZCB3aXRoIDB4N0Ygb3IgaW4gY2FzZSBvZiAw
eEZGIHRoZSBub2RlIERlZmF1bHQgVFRMIGZyb20gZGIgc2hvdWxkIGJlIHVzZWQuDQo+IA0KPiBJ
IHRoaW5rIHRoYXQgaXQgd2lsbCBiZSBoZWxwZnVsIGUuZy4gdG8gYXNzZXNzIHRoZSBiYXNpYyBy
ZWxpYWJpbGl0eSBvZiB0aGUgbWVzaCBuZXR3b3JrIGhhdmluZyB0aGUgaGVhcnRiZWF0IHB1Ymxp
Y2F0aW9uIFRUTCBhcyBhIHBhcmFtZXRlci4gVGhlbiwgZm9yIGV4YW1wbGUgeW91IGNhbiB2ZXJ5
IGVhc3kgYnVpbGQgdGhlIG1ldHJpYyB3aGljaCBkZXBlbmRzIG9uIFRUTC4NCg0KWW91IGFyZSBj
b3JyZWN0IHRoYXQgeW91IHNob3VsZCBuZXZlciBzZWUgYW55dGhpbmcgZ3JlYXRlciB0aGFuIDB4
N2Ygb3Zlci10aGUtYWlyLi4uLiBidXQgdGhlIGludGVudCB3YXMgdGhhdCB0aGUgbG93ZXIgbGF5
ZXJzIHdvdWxkIGhhbmRsZSB0aGUgdHJhbnNsYXRpb24gdG8gdGhlIHN5c3RlbSBzZXR0aW5nLi4u
LiBvciBpbiB0aGUgY2FzZSBvZiBoZWFydGJlYXQsIHRoZSBjb250cm9sIGxheWVyLg==

2018-03-23 11:20:00

by Robert Lubaś

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

Hi Brian,

On 03/22/2018 10:33 PM, Gix, Brian wrote:
> I am unable to look at all the code in context at the moment, but using the DEFAULT_TTL (0xff) for any TTL value, will automatically get replaced with whatever the default TTL has been set to. It is a handy value which means ?use the system setting? without needing to look it up.
>
>> On Mar 22, 2018, at 3:45 AM, Robert Luba? <[email protected]> wrote:
>>
>> In Mesh Profile spec 4.2.17.4 Heartbeat Publication TTL value range is
>> 0x00-0x7F. In cmd_hb_pub_set heartbeat ttl was set to DEFAULT_TTL 0xFF, this
>> patch fix this by adding ttl param to hb-pub-set.
>> ---
>> mesh/config-client.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/mesh/config-client.c b/mesh/config-client.c
>> index 19e617d62..0b5b8677b 100644
>> --- a/mesh/config-client.c
>> +++ b/mesh/config-client.c
>> @@ -1042,7 +1042,7 @@ static void cmd_hb_pub_set(int argc, char *argv[])
>> n = mesh_opcode_set(OP_CONFIG_HEARTBEAT_PUB_SET, msg);
>>
>> parm_cnt = read_input_parameters(argc, argv);
>> - if (parm_cnt != 5) {
>> + if (parm_cnt != 6) {
>> bt_shell_printf("Bad arguments: %s\n", argv[1]);
>> return bt_shell_noninteractive_quit(EXIT_FAILURE);
>> }
>> @@ -1056,12 +1056,12 @@ static void cmd_hb_pub_set(int argc, char *argv[])
>> /* Period Log */
>> msg[n++] = parms[2];
>> /* Heartbeat TTL */
>> - msg[n++] = DEFAULT_TTL;
>> + msg[n++] = parms[3];
>> /* Features */
>> - put_le16(parms[3], msg + n);
>> + put_le16(parms[4], msg + n);
>> n += 2;
>> /* NetKey Index */
>> - put_le16(parms[4], msg + n);
>> + put_le16(parms[5], msg + n);
>> n += 2;
>>
>> if (!config_send(msg, n)) {
>> @@ -1167,8 +1167,8 @@ static const struct bt_shell_menu cfg_menu = {
>> "Set relay"},
>> {"relay-get", NULL, cmd_relay_get,
>> "Get relay"},
>> - {"hb-pub-set", "<pub_addr> <count> <period> <features> <net_idx>",
>> - cmd_hb_pub_set, "Set heartbeat publish"},
>> + {"hb-pub-set", "<pub_addr> <count> <period> <ttl> <features> <net_idx>",
>> + cmd_hb_pub_set, "Set heartbeat publish"},
>> {"hb-pub-get", NULL, cmd_hb_pub_get,
>> "Get heartbeat publish"},
>> {"hb-sub-set", "<src_addr> <dst_addr> <period>",
>> --
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
I see your point. You know the mesh specification very well, but I will
just refer to it to construct the reply. The Mesh Profile specification
says: if the publish TTL is set to 0xFF the message use the Default TTL
(4.2.2.5). The Default TTL valid values are 0x00, 0x02-0x7F. Prohibited
are 0x01, 0x80-0x7FF (4.2.7).

However in terms of Heartbeat publication TTL the valid values are
0x00-0x7F. Prohibited are 0x80-0xFF (4.2.17.4). Taking into account
valid ranges the heartbeat TTL should be masked with 0x7F or in case of
0xFF the node Default TTL from db should be used.

I think that it will be helpful e.g. to assess the basic reliability of
the mesh network having the heartbeat publication TTL as a parameter.
Then, for example you can very easy build the metric which depends on TTL.

2018-03-22 21:33:56

by Gix, Brian

[permalink] [raw]
Subject: Re: [PATCH BlueZ V2] Mesh: Fix TTL in Config Heartbeat Publication Set

I am unable to look at all the code in context at the moment, but using the DEFAULT_TTL (0xff) for any TTL value, will automatically get replaced with whatever the default TTL has been set to. It is a handy value which means ?use the system setting? without needing to look it up.

> On Mar 22, 2018, at 3:45 AM, Robert Luba? <[email protected]> wrote:
>
> In Mesh Profile spec 4.2.17.4 Heartbeat Publication TTL value range is
> 0x00-0x7F. In cmd_hb_pub_set heartbeat ttl was set to DEFAULT_TTL 0xFF, this
> patch fix this by adding ttl param to hb-pub-set.
> ---
> mesh/config-client.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/mesh/config-client.c b/mesh/config-client.c
> index 19e617d62..0b5b8677b 100644
> --- a/mesh/config-client.c
> +++ b/mesh/config-client.c
> @@ -1042,7 +1042,7 @@ static void cmd_hb_pub_set(int argc, char *argv[])
> n = mesh_opcode_set(OP_CONFIG_HEARTBEAT_PUB_SET, msg);
>
> parm_cnt = read_input_parameters(argc, argv);
> - if (parm_cnt != 5) {
> + if (parm_cnt != 6) {
> bt_shell_printf("Bad arguments: %s\n", argv[1]);
> return bt_shell_noninteractive_quit(EXIT_FAILURE);
> }
> @@ -1056,12 +1056,12 @@ static void cmd_hb_pub_set(int argc, char *argv[])
> /* Period Log */
> msg[n++] = parms[2];
> /* Heartbeat TTL */
> - msg[n++] = DEFAULT_TTL;
> + msg[n++] = parms[3];
> /* Features */
> - put_le16(parms[3], msg + n);
> + put_le16(parms[4], msg + n);
> n += 2;
> /* NetKey Index */
> - put_le16(parms[4], msg + n);
> + put_le16(parms[5], msg + n);
> n += 2;
>
> if (!config_send(msg, n)) {
> @@ -1167,8 +1167,8 @@ static const struct bt_shell_menu cfg_menu = {
> "Set relay"},
> {"relay-get", NULL, cmd_relay_get,
> "Get relay"},
> - {"hb-pub-set", "<pub_addr> <count> <period> <features> <net_idx>",
> - cmd_hb_pub_set, "Set heartbeat publish"},
> + {"hb-pub-set", "<pub_addr> <count> <period> <ttl> <features> <net_idx>",
> + cmd_hb_pub_set, "Set heartbeat publish"},
> {"hb-pub-get", NULL, cmd_hb_pub_get,
> "Get heartbeat publish"},
> {"hb-sub-set", "<src_addr> <dst_addr> <period>",
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html