search

edgexfoundry-holding / device-bluetooth-c

Apache License 2.0
5 stars 4 forks source link

Please add build instructions #1

Open tonyespy opened 5 years ago

tonyespy commented 5 years ago

Please add build instructions to this project. Some open source projects provide build instructions via a top-level file called HACKING (or DEVELOPMENT, BUILD, ...):

You could also add the instructions to the top-level README.

The build instructions should mention:

FraserG01 commented 4 years ago

completed: edgexfoundry-holding/device-bluetooth-c/pull/12

tonyespy commented 4 years ago

@FraserG01 at last Monday's WG meeting, @iain-anderson asked me to review the some of the closed out issues in this device service that I'd reported. The changes made to the README are a definite step forward, but I think there's more work to be done here. Also, I realize that some of my comments below apply to more then just the build instructions, however it'd seemed easier to convey all of my comments about the current README in one shot. If you'd like me to break these non-build comments into a separate issue, I'd be glad to do so.

Global comments

Introduction

The BLE Device Service connects Bluetooth Low Energy devices with EdgeX. The Device Service currently limited to Bluetooth GATT devices/profiles.

  • suggest: This device service currently only supports the BLE GATT profile.

Prerequisites

A Linux build host.

  • Do we want to list distros/versions supported? tested?
  • suggest: State that this device service is only supported on Linux, as it requires both DBus and the bluez Bluetooth stack. Although it may be possible to get both running on MacOS or Windows, use on either is considered unsupported.

GCC that supports C11

  • suggest: gcc 5.x or greater (gcc 5 series defaults to -std=gnu11). Also note, -std=gnu11 is default, which means C11 plus GNU extensions, including those that conflict with C11; for strict C11, you need specify -std=c11. Is the default (gnu11) sufficient?
  • suggest: if not already present, add an assert for __STDC_VERSION__ == 201112L

Make 4.1

  • suggest: or greater

CMake 3.1 or greater

  • question: why do you need to specify both cmake and make minimum versions? The device-sdk-c only requires cmake 3 or greater, and doesn't stipulate a minimum version of make.

Cbor version 0.5

  • suggest: or greater
  • suggest: Is it OK for developers to use standard distro packages of this? Maybe mention that packages aren't available for your build host, see github for build instructions?
  • question: does this device service statically link against the library and/or require this library to be included in a container?
  • note: the last release of this library was 2017! Doesn't seem well supported.
  • question: cbor is already a dependency of the SDK, so why it include it here?

Device-SDK-C version 1.0.0 or greater.

  • note: the first version of device-sdk-c w/a top-level Makefile is v1.1.0, shouldn't we require v1.1.0 or better yet, the latest stable version v1.1.1?

D-Bus version 1.12.2

  • note: buildtime and/or runtime dependencies (see above)?
  • suggest: or greater
  • question: why 12.2? Ubuntu 16.04 includes dbus 1.10.6. Is there something specific required from dbus 1.12 or will it work with 1.10?

Bluez 5.48

  • note: buildtime and/or runtime dependencies (see above)?
  • note: libraries and dev packages are available on most distros; does this device service statically link against the library and/or require this library to be included in a container?
  • question: are there 5.48-specific features (mostly DBus API methods/interfaces) used by the DS? Has the DS been tested with 5.37 (default bluez on Ubuntu 16.04)

Build

Before building the device service, please ensure that you have the EdgeX Device-SDK-C installed

  • suggest: add a link on how to install Device-SDK-C
  • suggest: the repository needs to be re-named to device-ble-c, or you need to tell people to re-name when they clone. I suppose the other option is to just request the name change when you finally submit the PR to move this DS from holding to edgexfoundry.
  • note - my build fails due to lack of C SDK build (see below)

Run

Run the following command in shell to start the Device Service

  • question: is a shell required to run it?
tonyespy commented 4 years ago

Here's some separate but related feedback on device-sdk-c, based on my experience trying to build this device service on an Ubuntu 18.04.

FraserG01 commented 4 years ago

@tonyespy Here is my responses to your points raised, I have included the AppArmor disclaimer suggestion from https://github.com/edgexfoundry-holding/device-bluetooth-c/issues/4.

Also there is a pull request with these changes https://github.com/edgexfoundry-holding/device-bluetooth-c/pull/22. Please could you review it. Cheers, Fraser

I think you should just add a disclaimer to the top-level README that says it's only possible to run this device service fully confined on distros with AppArmor enabled (maybe listing a few), and that at this time, this device service is not supported on any distro with SE Linux enabled by default (I actually wonder if anyone has done a full test of EdgeX on SE Linux?). I'd also file a bug specifically for the issue that the service can't be run on an SE Linux enabled distro (even with --priviledged specified), and close this issue.

Global comments the term Device Service is capitalized everywhere, suggest lower case is more appropriate.

Introduction The BLE Device Service connects Bluetooth Low Energy devices with EdgeX. The Device Service currently limited to Bluetooth GATT devices/profiles.

Prerequisites Suggest: you should differentiate between build prerequisites (e.g. cmake, make, library dev packages or headers, ...) and runtime prerequisites (e.g. bluez, dbus). Note, some are both (i.e. libdbus-dev vs. libdbus.*.so).

A Linux build host. Do we want to list distros/versions supported? Tested?

suggest: State that this device service is only supported on Linux, as it requires both DBus and the bluez Bluetooth stack. Although it may be possible to get both running on MacOS or Windows, use on either is considered unsupported.

GCC that supports C11 suggest: gcc 5.x or greater (gcc 5 series defaults to -std=gnu11). Also note, -std=gnu11 is default, which means C11 plus GNU extensions, including those that conflict with C11; for strict C11, you need specify -std=c11. Is the default (gnu11) sufficient?

suggest: if not already present, add an assert for __STDC_VERSION__ == 20111

CMake 3.1 or greater question: why do you need to specify both cmake and make minimum versions? The device-sdk-c only requires cmake 3 or greater, and doesn't stipulate a minimum version of make.

Cbor version 0.5 suggest: Is it OK for developers to use standard distro packages of this? Maybe mention that if packages aren't available for your build host, see github for build instructions?

question: cbor is already a dependency of the SDK, so why it include it here?

question: does this device service statically link against the library and/or require this library to be included in a container?

note: the last release of this library was 2017! Doesn't seem well supported.

Device-SDK-C version 1.0.0 or greater.

D-Bus version 1.12.2 note: buildtime and/or runtime dependencies (see above)?

suggest: or greater

question: why 12.2? Ubuntu 16.04 includes dbus 1.10.6. Is there something specific required from dbus 1.12 or will it work with 1.10?

Bluez 5.48 note: buildtime and/or runtime dependencies (see above)?

note: libraries and dev packages are available on most distros; does this device service statically link against the library and/or require this library to be included in a container?

question: are there 5.48-specific features (mostly DBus API methods/interfaces) used by the DS? Has the DS been tested with 5.37 (default bluez on Ubuntu 16.04)

Build Before building the device service, please ensure that you have the EdgeX Device-SDK-C installed

suggest: the repository needs to be re-named to device-ble-c, or you need to tell people to re-name when they clone. I suppose the other option is to just request the name change when you finally submit the PR to move this DS from holding to edgexfoundry.

note - my build fails due to lack of C SDK build (see below)

Run suggest: service name should be device-ble, drop the -c

question: in v1.1.1 of device-sdk-go, --registry was updated from a boolean to a string arg which specifies the address of the registry. Was it updated in device-sdk-c too?

Run the following command in shell to start the Device Service question: is a shell required to run it?

Network Argument required for docker run (Raised by myself)

tonyespy commented 4 years ago

This is looking really good now. A couple of minor things still and this can get closed out.

Cbor version 0.5 suggest: Is it OK for developers to use standard distro packages of this? Maybe mention that if packages aren't available for your build host, see github for build instructions?

  • Cbor is both a runtime and build dependency, standard distribution packages are recommended

I still think it's weird that you list Cbor here, as it's really a dependency of the SDK, but I'm not going to block closing this over it. That said, you say it's both a build-time and a runtime dependency, but I'm assuming the latter is for the shared library, there's no CBor daemon, or other sort of runtime dependency for Cbor right? Please see my question about static vs. dynamic libraries. It sounds the resulting binary is linked against shared libraries that need to be installed/included in the host OS or container. This also confused me when I saw that you list CBor in the runtime dependencies. If the dependency really is the shared library, then why aren't the other shared libraries used by the SDK listed here? If you really want to keep it, then maybe add in parentheses that it's the library that's required at runtime.

Another option is to statically link the service...

note: libraries and dev packages are available on most distros; does this device service statically link against the library and/or require this library to be included in a container?

Neither as long as the host machine has the minimum version of bluez as we are just using the bluez daemon via dbus

For DBus, it looks like the runtime dependency is for both the shared library and a running system daemon.

FraserG01 commented 4 years ago

@tonyespy

I still think it's weird that you list Cbor here, as it's really a dependency of the SDK, but I'm not going to block closing this over it. That said, you say it's both a build-time and a runtime dependency, but I'm assuming the latter is for the shared library, there's no CBor daemon, or other sort of runtime dependency for Cbor right? Please see my question about static vs. dynamic libraries. It sounds the resulting binary is linked against shared libraries that need to be installed/included in the host OS or container. This also confused me when I saw that you list CBor in the runtime dependencies. If the dependency really is the shared library, then why aren't the other shared libraries used by the SDK listed here? If you really want to keep it, then maybe add in parentheses that it's the library that's required at runtime.

I spoke with @iain-anderson and he agrees that it can be removed from the dependencies. A note above the build requirements has been added which explains the SDK is required along with its dependencies instead, including CBor.

For DBus, it looks like the runtime dependency is for both the shared library and a running system daemon.

I have updated the requirements to reflect this.

Changes have been made in PR: https://github.com/edgexfoundry-holding/device-bluetooth-c/pull/26