Hid mouse prototype sample #10

kapi-no · 2018-06-19T07:25:25Z

This PR adds a prototype sample with HIDS mouse application. The sample acts as Bluetooth peripheral and uses HIDS library to implement the HID service. After connecting to the central, pressing keys will emulate mouse motion.

This sample is based on the example that is available in the nRF5 SDK.

So far, it has been tested with Android devices and Windows 7.

SebastianBoe · 2018-06-19T10:56:45Z

lib/Kconfig

+	  Enable ESB functionality
+
+if BLE
+source "../nrf/lib/ble/Kconfig"


It is best-practice to use

rsource "ble/Kconfig"

instead

SebastianBoe · 2018-06-19T10:57:03Z

lib/Kconfig

+	prompt "Enable BLE"
+	default n
+	help
+	  Enable ESB functionality


Help text does not match prompt.

SebastianBoe · 2018-06-19T10:58:17Z

lib/ble/Kconfig

+# SPDX-License-Identifier: BSD-5-Clause-Nordic
+#
+source "../nrf/lib/ble/common/Kconfig"
+source "../nrf/lib/ble/services/Kconfig"


Again, it is recommended to use 'rsource'.

SebastianBoe · 2018-06-19T10:59:34Z

lib/ble/common/Kconfig

+config BLE_SVC_COMMON
+	bool
+	prompt "Enable BLE service utils"
+	default n


"default n"

can and should be omitted. It will have the same semantics without.

SebastianBoe · 2018-06-19T11:03:39Z

samples/bluetooth/hids_mouse/CMakeLists.txt

+target_sources(app PRIVATE ${app_sources})
+
+zephyr_library_include_directories($ENV{ZEPHYR_BASE}/samples/bluetooth)
+zephyr_library_include_directories(src)


I can't see any reason why the source files in 'src' would need 'src' in their include path.

If they need to include a header file in 'src' they can simply do

#include "header_file_in_src.h"

I guess this might be needed when some of the source files are doing the "workaround hack" ...

SebastianBoe · 2018-06-19T11:05:53Z

samples/bluetooth/hids_mouse/src/dis.c

+ */
+
+/* Workaround build system bug that will put objects in source dir */
+#include "../../../zephyr/samples/bluetooth/gatt/dis.c"


You don't want to do this.

See how Emanuel avoided this in

#8 (comment)

kapi-no · 2018-06-19T13:48:29Z

@SebastianBoe,
I addressed your comments with fixes.
Please add more reviewers: @grochu, @lemrey, @pdunaj

SebastianBoe · 2018-06-19T13:51:31Z

lib/ble/CMakeLists.txt

+#
+add_library(nrf_ble INTERFACE)
+
+add_subdirectory(common)


Adding this directory should depend on BLE_SVC_COMMON

or else you might end up with a zephyr_library without a source file, which is not permitted.

To do this you can do

add_subdirectory_ifdef(CONFIG_BLE_SVC_COMMON common)

and remove the

_ifdef(CONFIG_BLE_SVC_COMMON

from the

zephyr_library_sources

invocation.

fixed for both: BLE_SVC_COMMON & BLE_HIDS

SebastianBoe

Build and configuration related changes are OK.

lemrey · 2018-06-19T15:09:41Z

@SebastianBoe or @carlescufi, could you please more reviewers?
I am available to review this pull request.

lemrey · 2018-06-20T07:32:48Z

It would be better to name the include folder
/include/bluetooth instead of /include/ble to align with what is already in zephyr.

pdunaj

Thanks, I hope this get in quick and we can start integrating the service.

One note to others:

there is no synchronization between notification and host query (i.e. multiple notifications can be in-flight while host query will return only the most recent value); this requires callback from gatt notification handler that data was sent and only one notification can be in-flight
we should not store mouse movements in the service (only absolute values should be recorded - for relative host should read zero).
API could be improved (it would be great if report map would be hidden and service would be initialized by giving an array of logical devices - buttons_device, axis_device, constructing map and maintaining ids of characteristics internally).
These were some things we have talked about already off-line.

pdunaj · 2018-06-20T06:15:13Z

lib/ble/services/hids.c

+
+#define BOOT_MOUSE_INPUT_REPORT_MIN_SIZE 3
+
+static ssize_t hids_protocol_mode_write(struct bt_conn            *conn,


alignment of arguments seems wrong

pdunaj · 2018-06-20T06:16:36Z

lib/ble/services/hids.c

+					u16_t offset,
+					u8_t flags)
+{
+	printk("%s\n", __func__);


I think it would be better if sys log is used instead.
RTT console can easily overflow if too many messages are printed so its nice to control what is logged.

I believe the logging subsystem is being re-written, @nordic-krch any comments?

Is it... I heard a rumor that CLI/Logger won't be scheduled for implementation in weeks or months.

logger is coming! :)

I am going to use sys_log for now, so its possible to suppress these debug messages (currently printk is used). Later on, I will port it to the new logger

@nordic-krch Do you know when? What about CLI?

pdunaj · 2018-06-20T06:20:17Z

include/ble/services/hids.h

+ */
+int hids_inp_rep_send(struct hids *hids_obj,
+		      u8_t rep_index,
+		      u8_t   *p_rep,


alignment seems wrong (in many cases its off by 1 char)

pdunaj · 2018-06-20T06:30:30Z

lib/ble/services/hids.c

+		&hids_obj->inp_rep_group.reports[rep_index];
+
+	if (p_hids_inp_rep->buff.size != len) {
+		return -1;


pdunaj · 2018-06-20T06:44:19Z

include/ble/services/hids.h

+	struct hids_info info;
+	struct hids_inp_rep_group inp_rep_group_init;
+	struct hids_rep_map rep_map;
+	bool is_mouse;


This structure lacks documentation. Especially is_mouse is not obvious. Will doxygen part be added later?

pdunaj · 2018-06-20T07:27:03Z

samples/bluetooth/hids_mouse/src/main.c

+static void mouse_movement_send(s16_t x_delta, s16_t y_delta)
+{
+	if (in_boot_mode) {
+		x_delta = min(x_delta, 0x00ff);


I am adding comment here but this is common for both modes.

First, this check should use defines from limits.h (SCHAR_MAX, SCHAR_MIN).
Second, you don't handle negative values.

pdunaj · 2018-06-20T07:29:43Z

samples/bluetooth/hids_mouse/src/main.c

+
+static struct device       *gpio_devs[4];
+static struct gpio_callback gpio_cbs[4];
+static bool		    in_boot_mode;


alignment (in many other places)

I will try to improve alignment in various places. However, in this case, if you view this file in normal (or raw) mode, it is aligned correctly. I am not sure why it is displayed this way here.

There are probably two reasons. First is that in this on-line viewer tab is 4 space long while Zephyr assumes 8. Second is that I guess you have strange mix of spaces and tabs. The easiest way to solve it would be to use only tabs or only spaces in such cases.

I pointed out the places where it looks the worst.
Anyway - we can make code pretty in subsequent changes :)

pdunaj · 2018-06-20T07:35:12Z

include/ble/services/hids.h

+ *  @param _name Name of the HIDS instace
+ */
+#define HIDS_DEF(_name)							     \
+	static struct bt_gatt_attr					     \


Could gatt attr array become part of the struct hids? Is there any reason to keep it separate?

Also, can we not hide the type of this variable and instead use xys_INITIALIZER macro (e.g. see _THREAD_INITIALIZER from kernel.h)?

pdunaj · 2018-06-20T07:36:02Z

include/ble/services/hids.h

+/** @def HIDS_DEF
+ *  @brief HIDS instance declaration macro.
+ *
+ *  @param _name Name of the HIDS instace


missing dot after instance

*instance typo :)

pdunaj · 2018-06-20T07:40:17Z

include/ble/services/hids.h

+};
+
+/* HIDS report type values. */
+enum {


Many of these enums and structures are actually private to the service. Could you consider moving them to the c file?

Keeping private elements in public files has disadvantages: it makes API less readable, private element can be misused and unnecessary code is preprocessed in all files where header is included.

lemrey · 2018-06-20T08:43:24Z

include/ble/services/hids.h

+
+/* HID Information descriptor. */
+struct hids_info {
+	u16_t bcd_hid;


As @pdunaj mentioned, please document the fields in these structures.

lemrey · 2018-06-20T08:52:00Z

lib/ble/services/hids.c

+
+	/* Register Report Map characteristic and its descriptor. */
+	memcpy(&hids_obj->rep_map, &hids_init_obj->rep_map,
+			sizeof(hids_obj->rep_map));


lemrey · 2018-06-20T09:00:45Z

samples/bluetooth/hids_mouse/src/main.c

+};
+
+
+static void hids_evt_handler(struct hids_evt *p_evt)


This is what we have been doing in the nRF5 SDK, but perhaps since zephyr implements callbacks slightly differently (see bt_conn_cb above for example), it could be worth considering whether we should do it differently in nRF Connect SDK.

Do you mean that we should use dedicated API for registering callbacks and individual callbacks for each event type?

lemrey · 2018-06-20T09:02:43Z

samples/bluetooth/hids_mouse/src/main.c

+
+static void hog_init(void)
+{
+	int err_code;


This naming "err_code" is not very much used in zephyr. I would suggest the shorter and more common "rc".

or just 'err'

lemrey · 2018-06-20T09:03:11Z

samples/bluetooth/hids_mouse/src/main.c

+	static u8_t buttons_buff[INPUT_REP_BUTTONS_LEN];
+	static u8_t movement_buff[INPUT_REP_MOVEMENT_LEN];
+	static u8_t media_buff[INPUT_REP_MEDIA_PLAYER_LEN];
+	static u8_t report_map[] = {


Is this a constant?

its constant now

lemrey · 2018-06-20T09:06:59Z

samples/bluetooth/hids_mouse/src/main.c

+	hids_init_obj.info.flags = (HIDS_REMOTE_WAKE |
+			HIDS_NORMALLY_CONNECTABLE);
+
+	p_hids_inp_rep = &hids_init_obj.inp_rep_group_init.reports[0];


There are no variable prefixes in zephyr, e.g. no p for pointers, although I like them, it would be good to align.
I have seen underscore prefixed static variables though e.g. "_var".

removed pointer prefixes.

"_" prefix for static variables is not always used, so I didn't rename my variables

lemrey · 2018-06-20T09:08:17Z

samples/bluetooth/hids_mouse/src/main.c

+
+void configure_buttons(void)
+{
+	static const u32_t pin_id[4] = { SW0_GPIO_PIN, SW1_GPIO_PIN,


Should these be static?

This was intended to be a per pin configuration. As such is static const.

kapi-no · 2018-06-20T09:10:59Z

samples/bluetooth/hids_mouse/src/main.c

+
+#include <gatt/dis.h>
+#include <gatt/bas.h>
+#include <ble/services/hids.h>


@lemrey suggested to change the name of includes/ble to includes/bluetooth for consistency.

This can be confusing when trying to find out whether the included header is part of zephyr (the same bluetooth directory) or nrf:

#include <bluetooth/hids.h> // NRF header
#include <bluetooth/uuid.h> // Zephyr header

Maybe we should use "nrf_" prefix for directories inside includes/. In this case:

#include <nrf_bluetooth/hids.h> // NRF header
#include <bluetooth/uuid.h> // Zephyr header

Any other suggestions/ideas are welcome.

Let's mention @carlescufi.

kapi-no · 2018-06-22T14:13:14Z

Update:

added sys_log to hids
changed location of BLE libraries from lib/ to subsys/ (as agreed during meeting)
structure fields are still undocumented (API may change in the next iterations)
various fixes from review

pdunaj · 2018-06-25T07:38:38Z

subsys/bluetooth/services/hids.c

+	SYS_LOG_DBG("Boot Mouse Input Report CCCD has changed.");
+
+	if (value == BT_GATT_CCC_NOTIFY) {
+		SYS_LOG_DBG("Notification for Boot Mouse has been turned on\n");


\n is not needed

pdunaj · 2018-06-25T07:42:39Z

This is by far the worst review tool I have ever used. I have no idea which defects I created are fixed or even are commented as I need to dig to get the comments from the author. There is no indication how many defects are open in total.

Does anybody else have the same feelings?

@carlescufi would it be possible to drop doing reviews here and switch to codecollaborator instead?

pdunaj · 2018-06-25T07:48:43Z

subsys/bluetooth/services/hids.c

+	buffer[len++] = hid_info->flags;
+
+	__ASSERT(len == HIDS_INFORMATION_CHAR_LEN,
+		 "HIDS Information encoding failed\n");


\n is not needed

pdunaj · 2018-06-25T07:49:16Z

subsys/bluetooth/services/hids.c

+}
+
+
+static u8_t hid_information_encode(u8_t			  *buffer,


is this alignment correct?

pdunaj · 2018-06-25T07:49:44Z

subsys/bluetooth/services/hids.c

+		evt.type = HIDS_EVT_HOST_EXIT_SUSP;
+		evt_handler(&evt);
+		break;
+


either remove this empty line or add one at line 227

pdunaj · 2018-06-25T07:50:34Z

subsys/bluetooth/services/hids.c

+static ssize_t hids_info_read(struct bt_conn            *conn,
+			      struct bt_gatt_attr const *attr,
+			      void                      *buf,
+			      u16_t len,


alignment of argument names is inconsistent

pdunaj · 2018-06-25T07:51:10Z

subsys/bluetooth/services/hids.c

+}
+
+
+static ssize_t hids_boot_mouse_inp_report_read(struct bt_conn            *conn,


inconsistent alignment or argument names

the same in other places

pdunaj · 2018-06-25T07:52:05Z

subsys/bluetooth/services/hids.c

+		evt.type = HIDS_EVT_REPORT_MODE_ENTERED;
+		evt_handler(&evt);
+		break;
+


excessive empty line

pdunaj · 2018-06-25T07:53:13Z

subsys/bluetooth/common/svc_common.c

+		break;
+
+	default:
+		__ASSERT(false, "Unknown UUID type\n");


\n not needed

pdunaj · 2018-06-25T07:53:26Z

subsys/bluetooth/common/svc_common.c

+static int uuid_register(struct bt_uuid      **const dest_uuid,
+			 struct bt_uuid const *const src_uuid)
+{
+	__ASSERT(*dest_uuid == NULL, "Overriding attribute UUID!\n");


\n not needed

also in other places (this is needed only with printk)

pdunaj · 2018-06-25T07:59:22Z

samples/bluetooth/hids_mouse/src/main.c

+	} else {
+		u8_t buffer[INPUT_REP_MOVEMENT_LEN];
+
+		x_delta = min(x_delta, 0x0fff);


The sanity check is incorrect - note that x_delta is signed. Please check as in line 321 (max(a,min(b,c)). I suggest connecting this with logical maximum/minimum from the report map.

pdunaj · 2018-06-25T08:05:52Z

samples/bluetooth/hids_mouse/src/main.c

+		x_delta = min(x_delta, 0x0fff);
+		y_delta = min(y_delta, 0x0fff);
+
+		buffer[0] = x_delta & 0x00ff;


maybe there is some macro that will convert to little endian whatever the endianess. If not can you check if memcpy would do the trick. In this second case please add static assert to check if endianess is as expected.
As this is sample somebody may compile it for different endianess configuration.

This commit adds HID service and its helper module to the ble library codebase. The module interacts with Zephyr bluetooth host layer to register HID service in the GATT database, pass notification data and respond to attribute callbacks. Signed-off-by: Kamil Piszczek <[email protected]>

This commit adds a sample with HIDS mouse application. The sample acts as peripheral and uses HIDS library to implement the HIDS service. After connecting to the central, pressing keys will emulate mouse motion. Signed-off-by: Kamil Piszczek <[email protected]>

pdunaj · 2018-06-27T08:42:16Z

Hi @SebastianBoe , can this be merged now or somebody else needs to approve?
If so could you do it?

-Using nRF Connect SDK fork pr for TF-M (nrfconnect#10) which adds settable BL2 configuration Signed-off-by: Frank Audun Kvamtrø <[email protected]>

…nrfconnect#10) Get timestamp from controller to allow drift compensation on gateway Signed-off-by: Alexander Svensen <[email protected]>

…#10) Get timestamp from controller to allow drift compensation on gateway Signed-off-by: Alexander Svensen <[email protected]>

-This manifest update and relevant commits in sdk-nrf is made to add support for nRF54H20 devices without a local build of the core -This includes taking in sdk-mbedtls pull request nrfconnect#15 for better client support in PK and MD wrapper APIs -This includes taking in sdk-oberon-psa-crypto pull request nrfconnect#10 for better client support as well as fixups for psa_util used for TLS/DTLS and X.509 testing Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This manifest update and relevant commits in sdk-nrf is made to add support for nRF54H20 devices without a local build of the core -This includes taking in sdk-mbedtls pull request nrfconnect#15 for better client support in PK and MD wrapper APIs -This includes taking in sdk-oberon-psa-crypto pull request nrfconnect#10 for better client support as well as fixups for psa_util used for TLS/DTLS and X.509 testing Ref: NCSDK-15632 Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This manifest update and relevant commits in sdk-nrf is made to add support for nRF54H20 devices without a local build of the core -This includes taking in sdk-mbedtls pull request nrfconnect#15 for better client support in PK and MD wrapper APIs -This includes taking in sdk-oberon-psa-crypto pull request nrfconnect#10 for better client support as well as fixups for psa_util used for TLS/DTLS and X.509 testing. Ref: NCSDK-15632 Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This manifest update and relevant commits in sdk-nrf is made to add support for nRF54H20 devices without a local build of the core -This includes taking in sdk-mbedtls pull request nrfconnect#15 for better client support in PK and MD wrapper APIs -This includes taking in sdk-oberon-psa-crypto pull request nrfconnect#10 for better client support as well as fixups for psa_util used for TLS/DTLS and X.509 testing Ref: NCSDK-15632 Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This manifest update and relevant commits in sdk-nrf is made to add support for nRF54H20 devices without a local build of the core -This includes taking in sdk-mbedtls pull request #15 for better client support in PK and MD wrapper APIs -This includes taking in sdk-oberon-psa-crypto pull request #10 for better client support as well as fixups for psa_util used for TLS/DTLS and X.509 testing Ref: NCSDK-15632 Signed-off-by: Frank Audun Kvamtrø <[email protected]>

SebastianBoe reviewed Jun 19, 2018

View reviewed changes

lib/Kconfig Outdated

prompt "Enable BLE"

default n

help

Enable ESB functionality

Copy link

Contributor

SebastianBoe Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help text does not match prompt.

SebastianBoe reviewed Jun 19, 2018

View reviewed changes

SebastianBoe requested changes Jun 19, 2018

View reviewed changes

SebastianBoe reviewed Jun 19, 2018

View reviewed changes

SebastianBoe approved these changes Jun 19, 2018

View reviewed changes

carlescufi requested review from pdunaj and lemrey June 19, 2018 15:20

pdunaj requested changes Jun 20, 2018

View reviewed changes

lemrey reviewed Jun 20, 2018

View reviewed changes

kapi-no commented Jun 20, 2018

View reviewed changes

pdunaj reviewed Jun 25, 2018

View reviewed changes

pdunaj requested changes Jun 25, 2018

View reviewed changes

kapi-no added 2 commits June 25, 2018 15:00

pdunaj approved these changes Jun 26, 2018

View reviewed changes

alexsven referenced this pull request in alexsven/sdk-nrf Jun 23, 2022

applications: nrf5340_audio: Get timestamp for gateway for drift-comp (…

320729c

…#10) Get timestamp from controller to allow drift compensation on gateway Signed-off-by: Alexander Svensen <[email protected]>

alexsven referenced this pull request in alexsven/sdk-nrf Jul 15, 2022

applications: nrf5340_audio: Get timestamp for gateway for drift-comp (…

3e96655

…#10) Get timestamp from controller to allow drift compensation on gateway Signed-off-by: Alexander Svensen <[email protected]>

divipillai pushed a commit to divipillai/sdk-nrf that referenced this pull request Jul 2, 2025

Add contribution guidelines (nrfconnect#10)

9418bc9


		#define BOOT_MOUSE_INPUT_REPORT_MIN_SIZE 3

		static ssize_t hids_protocol_mode_write(struct bt_conn *conn,

		}


		static ssize_t hids_boot_mouse_inp_report_read(struct bt_conn *conn,

Hid mouse prototype sample #10

Hid mouse prototype sample #10

Uh oh!

Conversation

kapi-no commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kapi-no commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SebastianBoe left a comment

Choose a reason for hiding this comment

Uh oh!

lemrey commented Jun 19, 2018

Uh oh!

lemrey commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdunaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lemrey commented Jun 20, 2018 •

edited

Loading