Return-Path: MIME-Version: 1.0 In-Reply-To: <2326994.RrINgRSL6X@athlon> References: <1413454646-23076-1-git-send-email-marcin.kraglak@tieto.com> <2326994.RrINgRSL6X@athlon> Date: Thu, 23 Oct 2014 10:55:09 +0300 Message-ID: Subject: Re: [PATCHv5 00/14] Included service discovery From: Luiz Augusto von Dentz To: Szymon Janc Cc: Arman Uguray , Marcin Kraglak , "linux-bluetooth@vger.kernel.org development" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Szymon, On Wed, Oct 22, 2014 at 9:39 PM, Szymon Janc wrote: > Hi, > > On Wednesday 22 October 2014 08:35:05 Arman Uguray wrote: >> Hi Luiz, >> >> On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz >> >> wrote: >> > Hi Marcin, >> > >> > On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak >> > >> > wrote: >> >> On 16 October 2014 12:17, Marcin Kraglak > wrote: >> >>> v3: >> >>> In this version after primary service discovery, >> >>> secondary services are discovered. Next included >> >>> services are resolved. With this approach we >> >>> don't have recursively search for included service, >> >>> like it was TODO in previous proposal. >> >>> There is also small coding style fix suggested by Arman. >> >>> >> >>> v4: >> >>> If no secondary services found, continue include services search (fixed >> >>> in gatt-client.c). >> >>> Fixed wrong debug logs (primary->secondary). >> >>> Fixed searching descriptors >> >>> >> >>> v5: >> >>> Ignore Unsupported Group Type Error in response to secondary service >> >>> discovery and continue included services discovery. >> >>> >> >>> Marcin Kraglak (14): >> >>> shared/gatt: Add discover_secondary_services() >> >>> shared/gatt: Add initial implementation of discover_included_services >> >>> shared/gatt: Discover included services 128 bit UUIDS >> >>> shared/gatt: Add extra check in characteristic iterator >> >>> shared/gatt: Add included service iterator >> >>> shared/gatt: Remove not needed function parameter >> >>> shared/gatt: Distinguish Primary from Secondary services >> >>> tools/btgatt-client: Print type of service >> >>> shared/gatt: Discover secondary services >> >>> shared/gatt: Discover included services >> >>> shared/gatt: Add gatt-client include service iterator >> >>> tools/btgatt-client: Print found include services >> >>> shared/gatt: Fix searching descriptors >> >>> shared/gatt: Add function bt_gatt_result_included_count() >> >>> >> >>> src/shared/gatt-client.c | 263 +++++++++++++++++++++++++++-- >> >>> src/shared/gatt-client.h | 18 ++ >> >>> src/shared/gatt-helpers.c | 418 >> >>> +++++++++++++++++++++++++++++++++++++++++++--- >> >>> src/shared/gatt-helpers.h | 10 +- >> >>> tools/btgatt-client.c | 17 +- >> >>> 5 files changed, 690 insertions(+), 36 deletions(-) >> >>> >> >>> -- >> >>> 1.9.3 >> > >> > Patches looks fine to me, but I would like Arman to ack before applying. >> >> I'm fine with the patches as they are, though I saw that Szymon has >> left comments on some of them (I'm guessing he's a doing a pass of the >> patch set now). I'm good with it as long as his comments are >> addressed. > > I really think we should have more comments and less magic numbers there. > Otherwise this code will be hard to maintain in long term. Yep, we could actually start creating packed structs for PDU as it was done for AVRCP so we could do sizeof etc, it makes the code much easier to understand and maintain. -- Luiz Augusto von Dentz