972248 - [B2F][NFC] : nfcd should using error code instead of boolean

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

Many API in NFC daemon now only return boolean to represent success or fail.
It would be better return error code instead of boolean

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Hi, I understand the error codes will be the ones defined in Bug 897312.

https://bugzilla.mozilla.org/attachment.cgi?id=831587

Comment 2

[Dimi Lee [:dimi]]

Attachment: WIP Patch v1

Hi Yoshi.
This is draft version of using error code, could you help check if you think the error code provided in this version is ok or you think nfcd should provide more detail error code. Thanks!

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8415161 [details] [diff] [review]
WIP Patch v1

Review of attachment 8415161 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/NfcGonkMessage.h
@@ +55,5 @@
> +  NFC_ERROR_INITIALIZE_FAIL = 8,
> +  NFC_ERROR_DEINITIALIZE_FAIL = 9,
> +  NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10,
> +  NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11,
> +  NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12,

CONNECT

@@ +57,5 @@
> +  NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10,
> +  NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11,
> +  NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12,
> +  NFC_ERROR_COULD_NOT_WRITE_TAG = 13,
> +  NFC_ERROR_COULD_NOT_MAKE_TAG_READ_ONLY = 14

Bug 897312 has already defined some error code, can we reuse them?

::: src/NfcService.cpp
@@ +65,5 @@
>  static sem_t thread_sem;
>  
> +#define CHECK_ERR_RTN(sta, err)  \
> +  if (sta) {  \
> +    mMsgHandler->processResponse(res, err, NULL); \

Not sure if this is a good macro,
1. We need to define 'res' when calling this macro, and the naming in Mozilla is usually .._ENSURE_...
2. Also usually we need to align \.
3. Usually we need to wrap it with do{} while(0) so caller can add semicolon and can prevent 
if (foo) 
  CHECK_ERR_RTN
else 
  ...
proble,

::: src/NfcUtil.h
@@ +10,5 @@
>  #include "NfcGonkMessage.h"
>  #include "TagTechnology.h"
>  
> +#define SUCCESSED(result)  (result == NFC_SUCCESS)
> +#define FAILED(result)  (result != NFC_SUCCESS)

These two macros doesn't bring much advantage.

If you'd like to use it, seems one is enough.

SUCCEED(result) ((result) == NFC_SUCCESS)

Comment 4

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #3)
> Comment on attachment 8415161 [details] [diff] [review]
> WIP Patch v1
> 
> Review of attachment 8415161 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +57,5 @@
> > +  NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10,
> > +  NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11,
> > +  NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12,
> > +  NFC_ERROR_COULD_NOT_WRITE_TAG = 13,
> > +  NFC_ERROR_COULD_NOT_MAKE_TAG_READ_ONLY = 14
> 
> Bug 897312 has already defined some error code, can we reuse them?
> 

I will re-use them in next version but still i'll add some more error code because this is not sufficient.

Comment 5

[Dimi Lee [:dimi]]

Attachment: NFCD use error code patch v1

Following Error code is modified:

NFC_ERROR_NFC_OFF = ‐18,
NFC_ERROR_NFC_DISCOVERY_ON = ‐19,
NFC_ERROR_NFC_DISCOVERY_OFF = ‐20,
NFC_ERROR_NFC_ENABLE_DISCOVERY = ‐21,
NFC_ERROR_NFC_DISABLE_DISCOVERY = ‐22,

Most of the error code value in  (v1.9) Nfcd/Gonk Protocol Specification is also changed.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8417222 [details] [review]
NFCD use error code patch v1

see comments on the github

Comment 7

[Dimi Lee [:dimi]]

Attachment: NFCD use error code patch v2

1.Update yoshi's review comment
2.Update following error code definition
* NFC_ERROR_NFC_ON -> NFC_ERROR_NFC_ALREADY_ON
* NFC_ERROR_NFC_OFF -> NFC_ERROR_NFC_ALREADY_OFF
* Add NFC_ERROR_ALREADY_DISCOVERY_ON
* Add NFC_ERROR_ALREADY_DISCOVERY_OFF
* Add NFC_ERROR_INITIALIZE_FAIL
* Add NFC_ERROR_DEINITIALIZE_FAIL

Comment 8

[Dimi Lee [:dimi]]

Attachment: NFCD use error code patch v2

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8417298 [details] [review]
NFCD use error code patch v2

basically ok, but I'd like to make code shorter and review this again.

Comment 10

[Dimi Lee [:dimi]]

Attachment: NFCD use error code patch v3

Update code according to yoshi's suggestion

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8417855 [details] [review]
NFCD use error code patch v3

see github for some nits

Comment 12

[Dimi Lee [:dimi]]

Attachment: Code refine for patch v3

Hi Yoshi,
  I have made some modification for Patch v3 according to suggestion, could you help review again ? Thanks.
  If you are ok with this, i will create another pull request

Comment 13

[Dimi Lee [:dimi]]

Attachment: pull request to mozilla-b2g/platform_system_nfcd

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/mozilla-b2g/platform_system_nfcd/commit/f620016437dd2f050e044eccef5e70e3f689ccbe