Support Bluetooth communcation JerryScript debugger #2633

kisbg · 2018-12-05T12:55:50Z

Documentation can be found in 07.DEBUGGER.md

JerryScript-DCO-1.0-Signed-off-by: Bence Gabor Kis kisbg@inf.u-szeged.hu

LaszloLango · 2019-01-09T14:56:31Z

jerry-core/CMakeLists.txt

 endif()

+if(FEATURE_BL_DEBUGGER)
+ set(FEATURE_DEBUGGER ON)


Wrong indentation: two spaces are needed

fixed the suggested changes

akosthekiss · 2019-01-14T17:43:25Z

.travis.yml

+        - sudo apt-get install -q libbluetooth-dev
+        - sudo apt-get install -q python-dev
+        - pip install --user pylint==1.6.5
+        - pip install --user pybluez


Why do code smell and style checks need the installation of additional system and python packages?

But if they are needed, the system dependencies should be added to addons.apt.packages.

akosthekiss · 2019-01-14T17:45:47Z

.travis.yml

+      install:
+        - sudo apt-get install -q libbluetooth-dev
+        - sudo apt-get install -q python-dev
+        - pip install --user pybluez


System dependencies should be listed under addons.apt.packages.

Plus, I think the separating empty line should not be removed from between the jobs.

akosthekiss · 2019-01-14T17:48:12Z

docs/07.DEBUGGER.md

+
+The following arguments must be passed to `tools/build.py`:
+
+`--jerry-bl-debugger=on`


I may be wrong but I cannot recall having seen BL as an abbreviation for Bluetooth. I've only seen BT for Bluetooth and BLE for Bluetooth Low Energy, but not BL.

akosthekiss · 2019-01-14T18:04:55Z

jerry-debugger/jerry_client.py

    try:
        main()
+    except bluetooth.BluetoothError as error_msg:
+        ERRNO = int(str(error_msg).split(",")[0].split("(")[1])


This string interpretation looks dangerous as well as superfluous.

BluetoothError is a subclass of IOError https://github.com/pybluez/pybluez/blob/master/bluetooth/btcommon.py#L179

socker.error is also a subclass of IOError https://docs.python.org/2/library/socket.html#socket.error , or of OSError https://docs.python.org/3.7/library/socket.html#socket.error

In Python 2, both IOError and OSError are subclasses of EnvironmentError, while in Python 3 they are aliases of each other. In both cases, they have an errno instance attribute.

So, a single except handler catching IOError as ... should be enough.

akosthekiss · 2019-01-14T18:17:48Z

jerry-debugger/jerry_client.py

+        MSG = str(error_msg)
+        if ERRNO == 111:
+            sys.exit("Failed to connect to the JerryScript debugger.")
+        elif ERRNO == 32 or ERRNO == 104:


I see that these numeric values are copy-pasted from existing code but are there no symbolic names for these error numbers in the errno module? https://docs.python.org/2/library/errno.html

akosthekiss · 2019-01-14T18:55:45Z

jerry-main/main-unix.c

  CLI_OPT_DEF (.id = OPT_DEBUG_SERVER, .longopt = "start-debug-server",
               .help = "start debug server and wait for a connecting client"),
+  CLI_OPT_DEF (.id = OPT_DEBUG_SERVER_TYPE, .longopt = "server-type", .meta = "TYPE",
+               .help = "choose a server type (choices : tcp or bluetooth) \


I'd suggest prefixing the cli option with debug- as well as the help message (i.e., "debug server type").

akosthekiss · 2019-01-14T18:56:56Z

jerry-main/main-unix.c

 #endif /* JERRY_ENABLE_EXTERNAL_CONTEXT */

-  init_engine (flags, start_debug_server, debug_port);
+  init_engine (flags, start_debug_server, debug_server_type,debug_port);


style: missing space after comma

akosthekiss · 2019-01-14T18:59:05Z

jerry-main/main-unix.c

+    }
+    else
+    {
+      printf ("bad argument (tcp or bluetooth)\n");


Command line argument validity check should not happen here at init_engine phase anymore. That should already be checked when arguments are processed around case OPT_DEBUG_SERVER_TYPE:.

akosthekiss · 2019-01-14T19:02:22Z

jerry-main/main-unix.c

 static void
 init_engine (jerry_init_flag_t flags, /**< initialized flags for the engine */
             bool debug_server, /**< enable the debugger init or not */
+             const char *debug_server_type, /**< choose which type or server create */


This argument shouldn't be a string. As all "message transmission interfaces" have the same prototype, I think this function should have a function pointer argument, something like: bool (*jerryx_debugger_transport_create) (uint16_t port). And the call site should pass either jerryx_debugger_tcp_create or jerryx_debugger_bluetooth_create.

I'd also prefer the function pointer argument.

akosthekiss · 2019-01-14T19:03:38Z

jerry-ext/include/jerryscript-ext/debugger.h


+/*
+ * Message transmission interfaces.
+ */


Don't repeat this comment. Just add the new declaration. The comment marks the beginning of a list. Only this list was one element long, until now.

rerobika · 2019-01-18T17:54:35Z

jerry-core/CMakeLists.txt


+# Enable bluetooth_debugger
+if(FEATURE_BL_DEBUGGER)
+  set(DEFINES_JERRY ${DEFINES_JERRY} JERRY_BL_DEBUGGER)


Cannot this define can be added the same if(FEATURE_BL_DEBUGGER) check above?

rerobika · 2019-01-18T17:56:17Z

jerry-ext/debugger/debugger-bluetooth.c

+#include "jerryscript-ext/debugger.h"
+#include "jext-common.h"
+
+#if defined (JERRY_DEBUGGER) && defined (JERRY_BL_DEBUGGER)


According to the jerry-core/CMakeLists.txt if the JERRY_BL_DEBUGGER is defined JERRY_DEBUGGER must be defined as well, so this check can be simplified.

rerobika · 2019-01-18T17:57:17Z

jerry-ext/debugger/debugger-bluetooth.c

+ */
+static bool
+jerryx_debugger_bluetooth_send (jerry_debugger_transport_header_t *header_p, /**< bluetooth implementation */
+                          uint8_t *message_p, /**< message to be sent */


Wrong indentation.

rerobika · 2019-01-18T18:01:13Z

jerry-ext/debugger/debugger-bluetooth.c

+    {
+      int err_val = errno;
+      jerry_debugger_transport_close ();
+      jerryx_debugger_bluetooth_log_error (err_val);


Each jerry_debugger_transport_close is followed by jerryx_debugger_bluetooth_log_error (err_val). How about adding the logging function to the end of the transport close function? Therefore it would be less function calls.

rerobika · 2019-01-18T18:02:48Z

jerry-ext/debugger/debugger-bluetooth.c

+  if (listen (server_socket, port) == -1)
+  {
+    close (server_socket);
+    jerryx_debugger_bluetooth_log_error(strerror (errno));


Missing spaces after each jerryx_debugger_bluetooth_log_error(strerror (errno)) function name.

rerobika · 2019-01-18T18:04:31Z

jerry-ext/debugger/debugger-bluetooth.c

+bool
+jerryx_debugger_bluetooth_create (uint16_t port)
+{
+  JERRYX_ERROR_MSG ("suppoert for Bluetooth debugging is disabled.\n");


Typo suppoert

rerobika · 2019-01-18T18:05:23Z

jerry-ext/debugger/debugger-bluetooth.c

+#else /* !JERRY_DEBUGGER || !JERRY_BL_DEBUGGER */
+
+/**
+ * Dummy function when debugger is disabluetoothd.


Kind of interesting word disabluetoothd, but probably a typo.

rerobika · 2019-01-18T18:05:49Z

jerry-ext/debugger/debugger-bluetooth.c

+                               0,
+                               JERRY_DEBUGGER_TRANSPORT_MAX_BUFFER_SIZE,
+                               0,
+                               JERRY_DEBUGGER_TRANSPORT_MAX_BUFFER_SIZE);


These 4 arguments are misaligned by 1 space.

rerobika · 2019-01-18T18:08:17Z

jerry-main/main-unix.c

 static void
 init_engine (jerry_init_flag_t flags, /**< initialized flags for the engine */
             bool debug_server, /**< enable the debugger init or not */
+             const char *debug_server_type, /**< choose which type or server create */


I'd also prefer the function pointer argument.

jerry-ext/debugger/debugger-bluetooth.c

kisbg · 2019-02-05T08:31:45Z

@akosthekiss @rerobika Thank you for the review!

rerobika · 2019-02-06T09:11:13Z

jerry-core/CMakeLists.txt

  set(FEATURE_LOGGING_MESSAGE " (FORCED BY STATS OR DUMP)")
 endif()

+


Unnecessary new line.

rerobika · 2019-02-06T09:11:32Z

docs/07.DEBUGGER.md

+- pip install --user pylint==1.6.5
+- pip install --user pybluez
+
+


Is the second new line is necessary?

rerobika · 2019-02-06T09:12:01Z

jerry-ext/CMakeLists.txt

+  set(DEFINES_EXT ${DEFINES_EXT} JERRY_BT_DEBUGGER)
+endif()
+
+


rerobika · 2019-02-06T09:12:56Z

jerry-ext/debugger/debugger-bluetooth.c

+bool
+jerryx_debugger_bt_create (uint16_t port) /**< listening port */
+{
+


rerobika · 2019-02-06T09:14:59Z

jerry-ext/debugger/debugger-bluetooth.c

+
+  if (port > 30)
+  {
+    JERRYX_DEBUG_MSG ("bluetooth port range is 1-30\n");


If the error message is correct, this check accepts the 0 port which is misleading.

rerobika · 2019-02-06T09:15:41Z

jerry-ext/debugger/debugger-bluetooth.c

+} /* jerryx_debugger_bt_create */
+
+#else /* !JERRY_BT_DEBUGGER */
+


Unnecessary new line.

rerobika · 2019-02-06T09:16:39Z

jerry-main/main-unix.c

+      {
+        if (check_feature (JERRY_FEATURE_DEBUGGER, cli_state.arg))
+        {
+          const char * temp = cli_consume_string (&cli_state);


const char * temp -> const char *temp

rerobika · 2019-02-06T09:17:54Z

jerry-main/main-unix.c

+          const char * temp = cli_consume_string (&cli_state);
+          if (strcmp (temp, "bluetooth") == 0)
+          {
+            debug_port = 1;


If the port is hard coded the check above for it could be an assert instead.

Documentation can be found in 07.DEBUGGER.md JerryScript-DCO-1.0-Signed-off-by: Bence Gabor Kis kisbg@inf.u-szeged.hu

kisbg force-pushed the bluetooth-szakdoga branch 8 times, most recently from 29e9714 to 1ec6114 Compare December 7, 2018 13:59

akosthekiss added the debugger Related to the debugger label Dec 13, 2018

kisbg force-pushed the bluetooth-szakdoga branch from 1ec6114 to 698d9e8 Compare December 20, 2018 13:20

kisbg changed the title ~~[WIP] Support bluetooth communcation Jerry debugger~~ Support Bluetooth communcation JerryScript debugger Dec 20, 2018

kisbg force-pushed the bluetooth-szakdoga branch 2 times, most recently from b64d000 to 1ddda76 Compare December 21, 2018 08:47

LaszloLango requested changes Jan 9, 2019

View reviewed changes

kisbg force-pushed the bluetooth-szakdoga branch from 1ddda76 to d7e045c Compare January 10, 2019 07:26

akosthekiss requested changes Jan 14, 2019

View reviewed changes

kisbg force-pushed the bluetooth-szakdoga branch from d7e045c to beb9720 Compare January 18, 2019 13:35

rerobika requested changes Jan 18, 2019

View reviewed changes

kisbg force-pushed the bluetooth-szakdoga branch 9 times, most recently from 5c44d06 to 8e3a2ec Compare January 28, 2019 08:24

rerobika requested changes Feb 6, 2019

View reviewed changes

Support Bluetooth communcation JerryScript debugger

6e34cfb

Documentation can be found in 07.DEBUGGER.md JerryScript-DCO-1.0-Signed-off-by: Bence Gabor Kis kisbg@inf.u-szeged.hu

kisbg force-pushed the bluetooth-szakdoga branch from 8e3a2ec to 6e34cfb Compare February 7, 2019 08:20

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

kisbg closed this Mar 20, 2019

kisbg deleted the bluetooth-szakdoga branch June 2, 2020 05:36


		The following arguments must be passed to `tools/build.py`:

		`--jerry-bl-debugger=on`

		set(FEATURE_LOGGING_MESSAGE " (FORCED BY STATS OR DUMP)")
		endif()

		- pip install --user pylint==1.6.5
		- pip install --user pybluez

		} /* jerryx_debugger_bt_create */

		#else /* !JERRY_BT_DEBUGGER */

Support Bluetooth communcation JerryScript debugger #2633

Support Bluetooth communcation JerryScript debugger #2633

Uh oh!

Conversation

kisbg commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kisbg Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

kisbg commented Feb 5, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kisbg commented Dec 5, 2018 •

edited

Loading

kisbg Jan 10, 2019 •

edited

Loading