Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5f6e9b95f48fd4c8488bfbbf85ca900db57dbf82.camel@ewol.com>
Subject: Re: [PATCH BlueZ v5 10/10] mesh: Refactor heartbeat pub/sub
From:   Steve Brown <sbrown@ewol.com>
To:     Inga Stotland <inga.stotland@intel.com>,
        linux-bluetooth@vger.kernel.org
Cc:     brian.gix@intel.com
Date:   Tue, 17 Nov 2020 07:25:38 -0500
In-Reply-To: <20200807013834.123263-11-inga.stotland@intel.com>
References: <20200807013834.123263-1-inga.stotland@intel.com>
         <20200807013834.123263-11-inga.stotland@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Inga,

This patch breaks heartbeat subscription set for me.

Subscription isn't enabled in net.c:mesh_net_set_heartbeat_sub() unless
sub->dst != dst. That is a change from prior to refactoring and seems
wrong.

Steve

On Thu, 2020-08-06 at 18:38 -0700, Inga Stotland wrote:
> Move heartbeat publication/subscription timers and housekeeping
> to net.c since this is where the trigger events and control messages
> are handled. Configuration server (cfgmod-server.c) stays
> responsible for parsing the set pub/sub message parameters and
> assemblying the pub/sub status messages.
> 
> Also, make sure that the correct message status is reported.
> ---
>  mesh/cfgmod-server.c | 307 ++++++++++++++---------------------------
> --
>  mesh/net.c           | 273 ++++++++++++++++++++++++++++----------
>  mesh/net.h           |  48 ++++---
>  3 files changed, 328 insertions(+), 300 deletions(-)
> 
> diff --git a/mesh/cfgmod-server.c b/mesh/cfgmod-server.c
> index ea0741969..ffeb0afeb 100644
> --- a/mesh/cfgmod-server.c
> +++ b/mesh/cfgmod-server.c
> 
------------- snip ----------------------------
> +               return 0;
>  
> -       if (hb->sub_dst != dst) {
> -               if (IS_GROUP(hb->sub_dst))
> -                       mesh_net_dst_unreg(net, hb->sub_dst);
> -               if (IS_GROUP(dst))
> -                       mesh_net_dst_reg(net, dst);
> -       }
> +       period_log = pkt[4];
>  
> -       hb->sub_enabled = !!period_log;
> -       hb->sub_src = src;
> -       hb->sub_dst = dst;
> -       hb->sub_count = 0;
> -       hb->sub_period = log_to_uint32(period_log, 1);
> -       hb->sub_min_hops = 0x00;
> -       hb->sub_max_hops = 0x00;
> +       if (period_log > 0x11)
> +               return 0;
>  
> -       gettimeofday(&time_now, NULL);
> -       hb->sub_start = time_now.tv_sec;
> +       net = node_get_net(node);
>  
> 
------------------ snip ------------------------

> diff --git a/mesh/net.c b/mesh/net.c
> index 9b4454fee..633727601 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> 

------------------- snip ----------------

> +
> +       } else if (sub->dst != dst) {
> +               if (IS_GROUP(sub->dst))
> +                       mesh_net_dst_unreg(net, sub->dst);
> +
> +               if (IS_GROUP(dst))
> +                       mesh_net_dst_reg(net, dst);
> +
> +               sub->enabled = !!period_log;
> +               sub->src = src;
> +               sub->dst = dst;
> +               sub->count = 0;
> +               sub->period = log_to_uint32(period_log);
> +               sub->min_hops = 0x00;
> +               sub->max_hops = 0x00;
> +               gettimeofday(&time_now, NULL);
> +               sub->start = time_now.tv_sec;
> +       }
> +

--------------------- snip --------------------