Re: [PATCH BlueZ] doc: Initial Bluetooth Mesh API

["Stotland, Inga" <inga.stotland@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

Hi Johan,

On Thu, 2018-11-08 at 12:45 +0200, Johan Hedberg wrote:
> Hi Inga,
> 
> At first glance this looks quite good to me, just some cosmetic
> comments:
> 
> > On 7 Nov 2018, at 2.49, Inga Stotland <inga.stotland@intel.com>
> > wrote:
> > +Service	org.bluez.mesh1
> 
> I don’t think we’ve had the habit of versioning well-known bus names
> so far. Or did you see this somewhere? So I’d just leave away the
> version here.
> 

From D-Bus Api Design Guidelines: (
https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning):

"In summary, version numbers should be included in all service names,
interface names and object paths:

    com.example.MyService1

    com.example.MyService1.InterestingInterface

    com.example.MyService1.OtherInterface

    /com/example/MyService1/Manager

    /com/example/MyService1/OtherObject
"

I chose to do the name versioning based on the official guidelines.
However, if we want to preserve the historical bluez approach, I don't
mind changing it.


> > +Interface	org.bluez.mesh1.Network
> 
> The way the existing interfaces do versioning is to have the version
> at the very end, so this should be org.bluez.mesh.Network1
> 
> > +Object path	/org/bluez/mesh1
> 
> I don’t think we have the habit of versioning object paths either.
> They’re anyway (for the most part) irrelevant - it’s the interfaces
> an object has that defines its type.
> 
> 
> > +			PossibleErrors:
> > +				org.bluez.mesh1.Error.Failed,
> > +				org.bluez.mesh1.Error.Canceled,
> > +				org.bluez.mesh1.Error.InvalidArguments
> > +				org.bluez.mesh1.Error.Timeout
> 
> We’ve never versioned errors either, or did you see this done
> somewhere? So just leave it out from here.

So you can see a number of services following the above mentioned D-Bus 
API Guidelines by running d-feet: e..g., org.freedesktop.GeoClue2 or
org.freedesktop.UDisks2 or org.fedoraprojectFirewallD1


> 
> > +		(object node, array{byte, array{(uint16, dict}}
> > configuration) Attach(
> > +									
> > object app_defined_root, uint64 token)
> > 
> 
> This is a bit hard to read due to the long & complex return value.
> I’d suggest doing the line break before “Attatch” (a common C coding-
> style as well).

Agree

> 
> > +		void Send(object element_path, uint16_t destination,
> > uint16 key_index,
> > +								array{b
> > yte} data)
> 
> Minor inconsistency: should be uint16 instead of uint16_t

Agree

> 
> Johan

Thank you for the review.

Regards,
Inga

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3266 bytes --]