Update BLE error calls to use new error codes and mbed_error #7726

SenRamakri · 2018-08-07T22:42:37Z

Description

This is to update BLE error() calls to use the new error handler methods, this enables use of error codes, produces more informative error report and also enables use of error decoding tools like Mbed-Error microsite - https://armmbed.github.io/mbedos-error/

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

cmonr

Would you mind separating the astyle changes with the error code changes into two commits?

SenRamakri · 2018-08-08T05:27:18Z

Astyle changes were unexpected, again some rebase issue. Let me fix that as well.

pan- · 2018-08-08T06:37:27Z

features/FEATURE_BLE/ble/services/EddystoneService.h

@@ -529,7 +529,7 @@ class EddystoneService
            DBG("attached tlmCallback every %d seconds", TlmAdvPeriod);
        }
        if (NONE == frameIndex) {
-            error("No Frames were Initialized! Please initialize a frame before starting an eddystone beacon.");
+            MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_BLE, MBED_ERROR_CODE_BLE_NO_FRAME_INITIALIZED), "No Frames were Initialized! Please initialize a frame before starting an eddystone beacon.");


I'm not sure there is value adding a new error for this specific case.
Would it be possible to use an INVALID_STATE error instead ?
We already have code such as INVALID_ARGUMENT.

@pan- The value here is to get maximum info about the error scenario just by decoding the error code even if there is no textual output. Please look at the error decoding microsite - https://armmbed.github.io/mbedos-error/. This website takes the error code and generates error context info. Once we integrate other features like device stats API for errors as well, the error-code info collected can be used to get more info. If you still think its not useful we can remove it. Let me know.

If the file and line number is reported, is it that useful to have very specific error code ? (If file and line are not accessible; it is very useful).
Do what feels right on this one :).

@pan- - The line number/file is not reported by default. It needs a special flag MBED_CONF_PLATFORM_ERROR_FILENAME_CAPTURE_ENABLED. Irrespective of that we plan to report these error-codes into cloud/or other backends and thus having these error codes would provide us more info for analytics/later analysis. Does that makes sense?

That's unfortunate it requires a special flag to report the filename and line number as without it the error code in this form is not very comprehensive.

Being dependent on a website to decipher an error code is, from my perspective, not ideal for debugging and it is also limited to the content we feed the website with. I hope their will be some way to run the webpage locally so it is in sync with the sources being run otherwise the whole error code mechanism will be very complex to use with application defined error codes.

Side question: how do you intend to capture in the cloud error codes reported when the system halt ?

pan- · 2018-08-08T06:38:36Z

features/FEATURE_BLE/source/BLE.cpp

@@ -141,7 +141,7 @@ BLE::initImplementation(FunctionPointerWithContext<InitializationCompleteCallbac

 // this stub is required by ARMCC otherwise link will systematically fail
 MBED_WEAK BLEInstanceBase* createBLEInstance() {
-    error("Please provide an implementation for mbed BLE");
+    MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_BLE, MBED_ERROR_CODE_BLE_NOT_IMPLEMENTED), "Please provide an implementation for mbed BLE");


In that specific case I would use the error code CREATE_FAILED.

Please change the error code to something like BLE_BACKEND_CREATION_FAILED.

pan- · 2018-08-08T06:44:06Z

features/FEATURE_BLE/source/BLE.cpp

@@ -191,7 +191,7 @@ void defaultSchedulingCallback(BLE::OnEventsToProcessCallbackContext* params) {
 bool BLE::hasInitialized(void) const
 {
    if (!transport) {
-        error("bad handle to underlying transport");
+        MBED_ERROR(MBED_MAKE_ERROR(MBED_MODULE_BLE, MBED_ERROR_CODE_BLE_BAD_TRANSPORT_HANDLE), "bad handle to underlying transport");


Same as above, I wold use a more generic INVALID_STATE code or better a NOT_INITIALIZED error code.
I would use the same code for all errors bellow.

The error code can be named BLE_BACKEND_NOT_INITIALIZED. The term transport present in the code is incorrect.

I'll fix that. Thanks.

SenRamakri · 2018-08-08T15:54:46Z

@cmonr and @pan- Rebase issue fixed and I have answered review comments, please review.

Changes addressed.

cmonr · 2018-08-09T01:08:07Z

@pan- Mind re-reviewing?

cmonr

LGTM

cmonr · 2018-08-14T01:48:06Z

@pan- Reminder that a re-review is waiting for you.

pan- · 2018-08-14T12:31:52Z

@SenRamakri @cmonr I've updated my comments.

cmonr · 2018-08-18T01:05:48Z

Looks like this PR will be parked until @SenRamakri gets back from vacation week after next.

SenRamakri · 2018-08-18T04:25:56Z

@pan- - Error code names have been updated, please review.

cmonr · 2018-08-29T15:11:27Z

@pan- Mind taking a nother look?

cmonr · 2018-08-29T15:11:37Z

@donatieng ^^^

NirSonnenschein · 2018-09-02T07:21:16Z

/morph build

mbed-ci · 2018-09-02T08:13:28Z

Build : SUCCESS

Build number : 2987
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7726/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-09-02T10:57:42Z

Exporter Build : SUCCESS

Build number : 2595
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7726/

mbed-ci · 2018-09-02T11:14:41Z

Test : FAILURE

Build number : 2745
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7726/2745

NirSonnenschein · 2018-09-02T11:34:25Z

/morph test

0xc0170 · 2018-09-02T18:28:09Z

/morph test

mbed-ci · 2018-09-02T22:01:22Z

Test : SUCCESS

Build number : 2750
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7726/2750

SenRamakri requested review from pan- and donatieng August 7, 2018 22:42

cmonr added the needs: review label Aug 8, 2018

cmonr previously requested changes Aug 8, 2018

View reviewed changes

pan- requested changes Aug 8, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 8, 2018

SenRamakri force-pushed the sen_BLE_ErrorUpdate branch from 8cd055d to 5dc61bc Compare August 8, 2018 15:47

cmonr approved these changes Aug 9, 2018

View reviewed changes

cmonr added needs: review and removed needs: work labels Aug 9, 2018

cmonr added the release-version: 5.10.0-rc1 label Aug 15, 2018

SenRamakri added 2 commits August 17, 2018 22:52

Update BLE error calls to use new error codes and mbed_error

7803a40

Changes to error code names

529af2b

SenRamakri force-pushed the sen_BLE_ErrorUpdate branch from 5dc61bc to 529af2b Compare August 18, 2018 04:24

cmonr added needs: work and removed needs: review labels Aug 30, 2018

pan- approved these changes Aug 31, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels Aug 31, 2018

cmonr merged commit 83e0e65 into ARMmbed:master Sep 3, 2018

0xc0170 removed the needs: CI label Sep 3, 2018

Update BLE error calls to use new error codes and mbed_error #7726

Update BLE error calls to use new error codes and mbed_error #7726

Uh oh!

Conversation

SenRamakri commented Aug 7, 2018

Description

Pull request type

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

SenRamakri commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Aug 14, 2018 • 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!

SenRamakri commented Aug 8, 2018

Uh oh!

cmonr commented Aug 9, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Aug 14, 2018

Uh oh!

pan- commented Aug 14, 2018

Uh oh!

cmonr commented Aug 18, 2018

Uh oh!

SenRamakri commented Aug 18, 2018

Uh oh!

cmonr commented Aug 29, 2018

Uh oh!

cmonr commented Aug 29, 2018

Uh oh!

NirSonnenschein commented Sep 2, 2018

Uh oh!

mbed-ci commented Sep 2, 2018

Triggering tests

Uh oh!

mbed-ci commented Sep 2, 2018

Uh oh!

mbed-ci commented Sep 2, 2018

Uh oh!

NirSonnenschein commented Sep 2, 2018

Uh oh!

0xc0170 commented Sep 2, 2018

Uh oh!

mbed-ci commented Sep 2, 2018

Uh oh!

Uh oh!

pan- Aug 14, 2018 •

edited

Loading