Implement PBAP connection related functions

RESOLVED FIXED in Firefox 41, Firefox OS v2.2r

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: btian, Assigned: btian)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S12 (15may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2r+, firefox41 fixed, b2g-v2.2r fixed)

Details

(Whiteboard: [red-tai])

Attachments

(6 attachments, 6 obsolete attachments)

10.52 KB, patch
Details | Diff | Splinter Review
14.73 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
8.31 KB, patch
Details | Diff | Splinter Review
10.24 KB, patch
Details | Diff | Splinter Review
5.27 KB, patch
Details | Diff | Splinter Review
14.73 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
TODO
- Listen PBAP socket as PSE & accept incoming connection
- Reply to PBAP client connection/disconnection request
(Assignee)

Comment 1

3 years ago
Created attachment 8598535 [details] [diff] [review]
WIP patch (v1)
Assignee: nobody → btian
(Assignee)

Comment 2

3 years ago
Created attachment 8599175 [details] [diff] [review]
Patch 1/3 (v1): Add OBEX related functions
Attachment #8598535 - Attachment is obsolete: true
Attachment #8599175 - Flags: review?(shuang)
(Assignee)

Comment 3

3 years ago
Created attachment 8599176 [details] [diff] [review]
Patch 2/3 (v1): Revise profile disconnection when BT stops
(Assignee)

Updated

3 years ago
Attachment #8599176 - Flags: review?(shuang)
(Assignee)

Comment 4

3 years ago
Created attachment 8599179 [details] [diff] [review]
Patch 3/3 (v1): Implement PBAP manger
Attachment #8599179 - Flags: review?(shuang)
Whiteboard: [red-tai]
Comment on attachment 8599175 [details] [diff] [review]
Patch 1/3 (v1): Add OBEX related functions

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

Overall code looks good.

::: dom/bluetooth/ObexBase.cpp
@@ +27,5 @@
>    return headerLength;
>  }
>  
>  int
> +AppendHeader(uint8_t aHeaderId, uint8_t* aRetBuf, int aValue)

Do you mind adding one line comment to explain the purpose of this function? After renaming this function, it's a bit hard to understand the purpose.

::: dom/bluetooth/ObexBase.h
@@ +209,5 @@
>        if (mHeaders[i]->mId == ObexHeaderId::Body ||
>            mHeaders[i]->mId == ObexHeaderId::EndOfBody) {
> +        uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetBody = new uint8_t[mHeaders[i]->mDataLength];
> +

nit:Remove this empty line.

@@ +229,3 @@
>          uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetTarget = new uint8_t[mHeaders[i]->mDataLength];
> +

nit:Remove this empty line.
Attachment #8599175 - Flags: review?(shuang) → review+
Comment on attachment 8599176 [details] [diff] [review]
Patch 2/3 (v1): Revise profile disconnection when BT stops

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +351,5 @@
> +    }
> +
> +    if (sProfiles[i]->IsConnected()) {
> +      sProfiles[i]->Disconnect(nullptr);
> +    } else if (!profileName.EqualsLiteral("OPP") &&

What would happen if someone modifies managers' name? Maybe it's not a real problem, but if the change happens it's not easy to find the side effects.
Attachment #8599176 - Flags: review?(shuang) → review+
(Assignee)

Comment 7

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> What would happen if someone modifies managers' name? Maybe it's not a real
> problem, but if the change happens it's not easy to find the side effects.

What do you mean by 'modify manager's name'? The name is queried on request [1] rather than a member variable. Also any suggestion? One way is to have empty |Reset| for OPP & PBAP managers.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothOppManager.h#46
Flags: needinfo?(shuang)
(Assignee)

Comment 8

3 years ago
Created attachment 8602609 [details] [diff] [review]
[final] Patch 1/3: Add OBEX related functions, r=shuang

Revise per reviewer's comment 5.
Attachment #8599175 - Attachment is obsolete: true
(In reply to Ben Tian [:btian] from comment #7)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > What would happen if someone modifies managers' name? Maybe it's not a real
> > problem, but if the change happens it's not easy to find the side effects.
> 
> What do you mean by 'modify manager's name'? The name is queried on request
> [1] rather than a member variable. Also any suggestion? One way is to have
> empty |Reset| for OPP & PBAP managers.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothOppManager.h#46

Keeping empty |Reset| seems to be trivial to me. Originally, I thought it's bad to check literal string (it depends on the assumption that no one is going to refactor the name) but now I think this is the easiest way. So please ignore my question.
Flags: needinfo?(shuang)
(Assignee)

Comment 10

3 years ago
Created attachment 8603267 [details] [diff] [review]
Patch 3/3 (v2): Implement PBAP manger

Revise |CompareHeaderTarget| function.
Attachment #8599179 - Attachment is obsolete: true
Attachment #8599179 - Flags: review?(shuang)
Attachment #8603267 - Flags: review?(shuang)
(Assignee)

Comment 11

3 years ago
Created attachment 8603981 [details] [diff] [review]
Patch 3/3 (v3): Implement PBAP manger

Changes:
- add functions |AfterPbapConnected| and |AfterPbapDisconnected| to set/reset member variables
- revise logs to be clear
Attachment #8603267 - Attachment is obsolete: true
Attachment #8603267 - Flags: review?(shuang)
Attachment #8603981 - Flags: review?(shuang)
Comment on attachment 8603981 [details] [diff] [review]
Patch 3/3 (v3): Implement PBAP manger

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

Code looks good, and it's very similar with OPP manager. Some nits need your help.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +11,5 @@
> +#include "BluetoothSocket.h"
> +#include "BluetoothUuid.h"
> +#include "ObexBase.h"
> +
> +//#include "mozilla/dom/bluetooth/BluetoothTypes.h"

nit: Please remove this line.

@@ +163,5 @@
> +
> +  mServerSocket =
> +    new BluetoothSocket(this, BluetoothSocketType::RFCOMM, false, true);
> +
> +  BT_LOGR("PBAP");

nit: Maybe you can have just one single message with a descriptive string.

@@ +186,5 @@
> +  const uint8_t* data = aMessage->GetData();
> +  int receivedLength = aMessage->GetSize();
> +  uint8_t opCode = data[0];
> +
> +  BT_LOGR("> 0x%x", opCode);

If the opCode won't create much trouble, I prefer don't print it.

@@ +208,5 @@
> +
> +      ReplyToConnect();
> +      AfterPbapConnected();
> +      break;
> +

nit: Please remove this blank line.

@@ +222,5 @@
> +
> +      ReplyToDisconnectOrAbort();
> +      AfterPbapDisconnected();
> +      break;
> +

nit: Please remove this blank line.

@@ +231,5 @@
> +    case ObexRequestCode::SetPath:
> +      ReplyError(ObexResponseCode::BadRequest);
> +      BT_LOGR("Unsupported ObexRequestCode %x", opCode);
> +      break;
> +

nit: Please remove this blank line.

@@ +350,5 @@
> +
> +void
> +BluetoothPbapManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize)
> +{
> +  BT_LOGR("< 0x%x", aOpcode);

Do we really need to print opcode?

@@ +362,5 @@
> +  MOZ_ASSERT(aSocket);
> +  MOZ_ASSERT(aSocket == mServerSocket);
> +  MOZ_ASSERT(!mSocket);
> +
> +  BT_LOGR("PBAP");

Ditto.
Attachment #8603981 - Flags: review?(shuang)
(Assignee)

Comment 13

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> @@ +186,5 @@
> > +  const uint8_t* data = aMessage->GetData();
> > +  int receivedLength = aMessage->GetSize();
> > +  uint8_t opCode = data[0];
> > +
> > +  BT_LOGR("> 0x%x", opCode);
> 
> If the opCode won't create much trouble, I prefer don't print it.
> 
> @@ +350,5 @@
> > +
> > +void
> > +BluetoothPbapManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize)
> > +{
> > +  BT_LOGR("< 0x%x", aOpcode);
> 
> Do we really need to print opcode?

I keep them for later development debugging. I'll remove in next patch.
(Assignee)

Comment 14

3 years ago
Created attachment 8604009 [details] [diff] [review]
[final] Patch 3/3: Implement PBAP manger, r=shuang
Attachment #8603981 - Attachment is obsolete: true
Attachment #8604009 - Flags: review?(shuang)
Comment on attachment 8604009 [details] [diff] [review]
[final] Patch 3/3: Implement PBAP manger, r=shuang

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +265,5 @@
> +
> +void
> +BluetoothPbapManager::AfterPbapConnected()
> +{
> +  mConnected = true;

Do you plan to add more pieces of code in the future? I guess one line function is not necessary.

@@ +271,5 @@
> +
> +void
> +BluetoothPbapManager::AfterPbapDisconnected()
> +{
> +  mConnected = false;

Ditto.
Attachment #8604009 - Flags: review?(shuang) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> Do you plan to add more pieces of code in the future? I guess one line
> function is not necessary.

Yes, please refer to bug 1162902. I'll do some cleanup once all implementation are done.
(Assignee)

Comment 17

3 years ago
ICS emulator fails to build since PBAP manager is unsupported for BlueZ. Revising patch and retry.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acc5dc6c76e
(Assignee)

Comment 18

3 years ago
Created attachment 8605119 [details] [diff] [review]
[final] Patch 2/3: Revise profile disconnection when BT stops, r=shuang

Add build flag to build for BlueZ on ICS emulator.
Attachment #8599176 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8604009 - Attachment description: Patch 3/3 (v4): Implement PBAP manger → [final] Patch 3/3: Implement PBAP manger, r=shuang
(Assignee)

Comment 19

3 years ago
Try with comment 18 patch. All builds pass.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23642f845ebd

Comment 20

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/83a9963ed83e
https://hg.mozilla.org/integration/b2g-inbound/rev/167fb3ad5f71
https://hg.mozilla.org/integration/b2g-inbound/rev/79e5169adc16
https://hg.mozilla.org/mozilla-central/rev/83a9963ed83e
https://hg.mozilla.org/mozilla-central/rev/167fb3ad5f71
https://hg.mozilla.org/mozilla-central/rev/79e5169adc16
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
(Assignee)

Comment 22

2 years ago
Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.
Flags: needinfo?(whuang)
(Assignee)

Comment 23

2 years ago
Created attachment 8650326 [details] [diff] [review]
[2.2r] Patch 1/3: Add OBEX related functions, r=shuang
(Assignee)

Comment 24

2 years ago
Created attachment 8650329 [details] [diff] [review]
[2.2r] Patch 2/3: Revise profile disconnection when BT stops, r=shuang
(Assignee)

Comment 25

2 years ago
Created attachment 8650330 [details] [diff] [review]
[2.2r] Patch 3/3: Implement PBAP manger, r=shuang
(Assignee)

Updated

2 years ago
Blocks: 1162902
(Assignee)

Updated

2 years ago
Depends on: 1152694
feature-b2g: --- → 2.2r+
status-b2g-v2.2r: --- → affected
Flags: needinfo?(whuang)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/60060b904a77
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/72e8518103d4
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/e0c17434760f
status-b2g-v2.2r: affected → fixed
You need to log in before you can comment on or make changes to this bug.