1230032 - [OTA] [NFC] [META] Separate Gecko and Gonk layers in NFC module

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: NFC, defect)

Description

[Ethan Tseng [:ethan]]

This bug is used to track the plan to refactor NFC module for the OTA project.

Comment 1

[Ethan Tseng [:ethan]]

Attachment: NFC_diagram_0.1.png

This is the architecture diagram of NFC module [1].

NFC already implements a NFC daemon (nfcd), which separates Gecko and Gonk layers and hides hardware dependencies. Gecko and Gonk communicates via a JSON based protocol.

It means the current design of NFC almost conforms to the proposed Gonk architecture of OTA project. We don't expect much change in NFC for the OTA project.


[1] Origin: https://wiki.mozilla.org/Security/Reviews/B2G/WebNFC

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for looking at this. Having worked a bit on the low-level NFC code, I think NFC is mostly in good shape. There are some calls to set Androids properties, which should probably be hidden within the HAL module. And I think NFC uses Binder Parcels internally. It should be possible to replace them with something that inherits from |UnixSocketBuffer| [1].

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/unixsocket/SocketBase.h#27

Comment 3

[Thomas Nguyen (:tnguyen)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> Thanks for looking at this. Having worked a bit on the low-level NFC code, I
> think NFC is mostly in good shape. There are some calls to set Androids
> properties, which should probably be hidden within the HAL module. And I

Do you mean creating a new HAL module to call the properties, then other features will be able to use? Could you please tell more details.
In my opinion, because only NFC has to use the properties set, probably the properties set could be hidden in a shared library (source code is located in same folder of deamon source system/nfcd)
> think NFC uses Binder Parcels internally. It should be possible to replace
> them with something that inherits from |UnixSocketBuffer| [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/ipc/unixsocket/SocketBase.h#27
Both nfcd daemon and NFC (gecko) are using Parcel so we have to change in both side. Nfcd daemon could not use Gecko's UnixSocketBuffer, I would prefer to create a new buffer for that ipc.
Thanks for your help.

Comment 4

[Thomas Nguyen (:tnguyen)]

Clear ni. Found information from bug 1233622

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: WIP NFC gecko patch

WIP NFC gecko patch

Comment 6

[Thomas Nguyen (:tnguyen)]

Attachment: WIP nfcd patch

WIP nfcd patch

Comment 7

[Thomas Nguyen (:tnguyen)]

Depends on 1233622 because NFC use ro.moz.nfc.enabled to Enable/Disable mozNfc at runtime

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8702453 [details] [diff] [review]
WIP nfcd patch

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

Thanks!
Please ask for feedback again after comment addressed

::: Android.mk
@@ +37,5 @@
>      src/snep/SnepMessage.cpp \
>      src/snep/SnepMessenger.cpp \
>      src/handover/HandoverClient.cpp \
>      src/handover/HandoverServer.cpp \
> +    src/IpcSocketBuffer.cpp \

I guess libbinder can be removed

::: src/IpcSocketBuffer.cpp
@@ +3,5 @@
> +
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Sync with other files in nfcd

@@ +8,5 @@
> +
> +#include <algorithm>
> +#include <errno.h>
> +#include <string.h>
> +#include <unistd.h>

Do we need <errno.h>, <string.h>, <unistd.h> ?

::: src/IpcSocketBuffer.h
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

Sync with other header files in nfcd

@@ +3,5 @@
> +
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Sync with other header files in nfcd

@@ +21,5 @@
> + */
> +class IpcSocketBuffer
> +{
> +public:
> +  virtual ~IpcSocketBuffer();

Do we need virtual here?

::: src/MessageHandler.cpp
@@ +27,3 @@
>  
>  #define MAJOR_VERSION (1)
>  #define MINOR_VERSION (22)

We need update this whenever protocol change, i'll suggest update MAJOR_VERSION for this bug

@@ +90,3 @@
>    int32_t sizeLe, size, request;
>    uint32_t status;
> +  std::auto_ptr<IpcSocketBuffer> buffer(new IpcSocketBuffer(aDataLen));

I don't see the reason using pointer here.
Maybe just
IpcSocketBuffer buffer = IpcSocketBuffer(aDataLen);

If you agree with this, please also change function using IpcSocketBuffer from pointer to reference/const reference

@@ +198,3 @@
>  {
> +  int32_t rfState;
> +  aBuffer->Read(rfState);

Nits. extra line

@@ +216,3 @@
>  {
> +  int32_t sessionId;
> +  aBuffer->Read(sessionId);

ditto.

@@ +225,5 @@
>    NdefMessagePdu ndefMessagePdu;
>    NdefMessage* ndefMessage = new NdefMessage();
>  
> +  int32_t sessionId;
> +  aBuffer->Read(sessionId);

ditto.

@@ +395,3 @@
>  
>    // is ready only
> +  aBuffer->Write(aInfo->isReadOnly);

Fix compile warning

@@ +398,3 @@
>  
>    // ndef formatable
> +  aBuffer->Write(aInfo->isFormatable);

ditto.

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8702452 [details] [diff] [review]
WIP NFC gecko patch

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

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +13,5 @@
>  #include <android/log.h>
>  #define NMH_LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "NfcMessageHandler", args)
>  
>  #define NFCD_MAJOR_VERSION 1
>  #define NFCD_MINOR_VERSION 22

update MAJOR version

@@ +263,2 @@
>  {
> +  int32_t techCount, idCount, ndefMsgCount, ndefInfo;

declare when used

@@ +340,3 @@
>  
>    for (int i = 0; i < recordCount; i++) {
> +    int32_t tnf, typeLength, idLength, payloadLength;

ditto.

::: dom/nfc/gonk/NfcMessageHandler.h
@@ +6,5 @@
>  
>  #ifndef NfcMessageHandler_h
>  #define NfcMessageHandler_h
>  
> +#include "mozilla/ipc/SocketBase.h"

use forward declaration

@@ +45,5 @@
> +  bool NDEFReceivedNotification(UnixSocketBuffer* aBuffer, EventOptions& aOptions);
> +
> +  bool ReadNDEFMessage(UnixSocketBuffer* aBuffer, EventOptions& aOptions);
> +  bool WriteNDEFMessage(UnixSocketBuffer* aBuffer, const CommandOptions& aOptions);
> +  bool ReadTransceiveResponse(UnixSocketBuffer* aBuffer, EventOptions& aOptions);

use const pointer for those read-only functions

::: dom/nfc/gonk/NfcService.cpp
@@ +333,5 @@
>    MOZ_ASSERT(mHandler);
>    MOZ_ASSERT(aBuffer);
>  
>    while (aBuffer->GetSize()) {
> +    EventOptions event;

declare before used

Comment 10

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

Sorry, I think these patches are off-topic.

The purpose of this bug should be Gecko still can run after OTA without crashing,
but from the patches, it looks it tries to reduce the dependencies on Android.
These seems two seperate problems to me.

I don't know if there are some offline discussions here, but if there are, or the direction of this bug is changed, those information should be made public on bugzilla.

(In reply to Dimi Lee[:dimi][:dlee] from comment #9)
> ::: dom/nfc/gonk/NfcMessageHandler.cpp
> @@ +13,5 @@
> >  #include <android/log.h>
> >  #define NMH_LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "NfcMessageHandler", args)
> >  
> >  #define NFCD_MAJOR_VERSION 1
> >  #define NFCD_MINOR_VERSION 22
> 
> update MAJOR version
> 
This is wrong, that's the version of the IPC protocol, not the version of implementation.
If you think the naming is confusing, file a bug and fix it.

But if you do trying to modify the protocol version here, that should be well discussed first, otherwise you will breaking foxfooding.

Comment 11

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #9)
> > ::: dom/nfc/gonk/NfcMessageHandler.cpp
> > @@ +13,5 @@
> > >  #include <android/log.h>
> > >  #define NMH_LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "NfcMessageHandler", args)
> > >  
> > >  #define NFCD_MAJOR_VERSION 1
> > >  #define NFCD_MINOR_VERSION 22
> > 
> > update MAJOR version
> > 
> This is wrong, that's the version of the IPC protocol, not the version of
> implementation.
> If you think the naming is confusing, file a bug and fix it.
> 
> But if you do trying to modify the protocol version here, that should be
> well discussed first, otherwise you will breaking foxfooding.
Yes, you are right, the protocol doesn't change.
I thought it is changed when review... sorry for the mistake.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8702453 [details] [diff] [review]
WIP nfcd patch

I don't know if that is still valid, but basic Gonk code including nfcd used the Apache License. Please don't just copy code from Gecko without considering licensing issues.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #3)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> > Thanks for looking at this. Having worked a bit on the low-level NFC code, I
> > think NFC is mostly in good shape. There are some calls to set Androids
> > properties, which should probably be hidden within the HAL module. And I
> 
> Do you mean creating a new HAL module to call the properties, then other
> features will be able to use? Could you please tell more details.
> In my opinion, because only NFC has to use the properties set, probably the
> properties set could be hidden in a shared library (source code is located
> in same folder of deamon source system/nfcd)

We have hal/gonk to contain such calls to Android APIs.

NFC uses |NFC_ENABLED = libcutils.property_get("ro.moz.nfc.enabled", "false")|. That call to |property_get| should be hidden by such an API. But |property_get| is a blocking call on the main thread. Some ideas a needed to turn it into a non-blocking API. 

> > think NFC uses Binder Parcels internally. It should be possible to replace
> > them with something that inherits from |UnixSocketBuffer| [1].
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/ipc/unixsocket/SocketBase.h#27
> Both nfcd daemon and NFC (gecko) are using Parcel so we have to change in
> both side. Nfcd daemon could not use Gecko's UnixSocketBuffer, I would
> prefer to create a new buffer for that ipc.
> Thanks for your help.

A few comments

1) Please respect the licensing terms of nfcd as mentioned above.

2) IIRC Parcel has some alignment issues in its implmentation. So this will break the interface between Gecko and nfcd if you land it. Foxfooders will require a FOTA to get NFC working again.

2) |UnixSocketBuffer| is possible but not optimal, because it returns a sequence of characters. Since you're going to break the protocol, you should instead build upon the code in hal/daemon. It implements sockets on top of SOCK_SEQPACKET, which gives you individual messages one by one. So no more searching for EOMs in the byte stream.

Comment 14

[Thomas Nguyen (:tnguyen)]

Thanks Thomas for your comments.

> 2) IIRC Parcel has some alignment issues in its implmentation. So this will
> break the interface between Gecko and nfcd if you land it. Foxfooders will
> require a FOTA to get NFC working again.
 Exactly. There's a padding alignment added if we write to a parcel, so removing usage of parcel requires modification in both nfcd and gecko. FOTA certainly will be required to get NFC working ( or a full flash )
I'm a little bit confused because, AFAIK, fota is not ready now. Is it necessary that new changes of nfcd and gecko are compatible with the old interface (using parcel)?

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #14)
> Thanks Thomas for your comments.
> 
> > 2) IIRC Parcel has some alignment issues in its implmentation. So this will
> > break the interface between Gecko and nfcd if you land it. Foxfooders will
> > require a FOTA to get NFC working again.
>  Exactly. There's a padding alignment added if we write to a parcel, so
> removing usage of parcel requires modification in both nfcd and gecko. FOTA
> certainly will be required to get NFC working ( or a full flash )
> I'm a little bit confused because, AFAIK, fota is not ready now. Is it
> necessary that new changes of nfcd and gecko are compatible with the old
> interface (using parcel)?

No, not yet. After Parcel is gone, we could think about protocol versions and transition phases. I don't know what NFC already provides.

I was thinking about removing Parcel from the NFC code. I'm happy to see this change happening BTW.

Maybe you want to coordinate with Naoki (?) for the FOTA updates, so that there's a FOTA update closely after you landed the changes.

Comment 16

[Thomas Nguyen (:tnguyen)]

I think removing usage of Parcel is out of topic. Filed a new bug for that
Bug 1236765 - [NFC] Remove/reduce Android dependencies in gecko/gonk

Comment 17

[Ethan Tseng [:ethan]]

(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> Sorry, I think these patches are off-topic.
> The purpose of this bug should be Gecko still can run after OTA without
> crashing,
> but from the patches, it looks it tries to reduce the dependencies on
> Android.
> These seems two separate problems to me.

Thanks Yoshi for pointing out this perspective.

We clarified this issue in an OTA project meeting today.
Removing the dependencies on Android is part of the future of Gonk, which should benefit the OTA project. So, let's treat this bug as a meta bug to trace all NFC bugs for the OTA project.

p.s.If anyone comes out a better title for this bug, go ahead to change it :)