Conversation
|
Only looked at this for 2 seconds so far, but doesnt appear to work on osx (yosemite) |
|
Unless something has changed I thought connects don't work on OSx |
|
Also the |
| "<div>Adding more content to see if I can it over the MTU value</div>" + | ||
| "</body>" + | ||
| "</html>"; | ||
|
|
There was a problem hiding this comment.
What do you think about adding this in a separate file and using the fs module to read the contents?
|
@hardillb nice work!
This might be ok with Yosemite since the @jacobrosenthal noticed the MTU was negotiated to 505 bytes, which seems a bit high ... I think the max according to spec is 256 bytes. |
|
Everything looks fine to me as far as FatBeacon is concerned. I have to admit I am not a Node expert though. |
lib/HTMLCharacteristic.js
Outdated
| HTMLCharacteristic.prototype.onReadRequest = function(offset, callback) { | ||
| //var buf = new Buffer(this._html, 'utf8'); | ||
| if (queueOffset < this._buffer.length) { | ||
| var transfer = currentMTU - 5; |
There was a problem hiding this comment.
@hayesjordan can you comment on why physical web is using -5 instead of -3 overhead?
There was a problem hiding this comment.
There is no specific reason for using -5, it can be anything less than 0. The only reason for it is to by pass the Android OS on the client device.
There was a problem hiding this comment.
Its currently oddly implemented on the android side. Transfer is defaulted to 20 (which would be default (23) -3 as Id expect, but then when mtu changes its mtu (505)-5 Seems like if you wanted it to be mtu-5 then transfer defined above there should be 18 not 20 for consistancy.. though it seems like it should be the size of the BLE spec overhead, which I think is 3.
There was a problem hiding this comment.
I was not aware of a BLE spec for overhead, but the numbers didn't seem to be important at the time. I was only trying to keep them for easy multiples when I was debugging this is.
There was a problem hiding this comment.
I come from the embedded world more, and I know 20 bytes of the 23 is typical for the nordic devices, I presume this is in the spec, perhaps its their limitations. Ill keep researching and maybe @sandeepmistry can illuminate
There was a problem hiding this comment.
Looks like as low as 1 actually. Ive heard 20 quoted a ton on nrf, wonder if thats correct over there.
3.4.7.1 Handle Value Notification
Attribute Value
0 to (ATT_MTU–3)
3.4.4.4 Read Response
0 to (ATT_MTU–1)
There was a problem hiding this comment.
@jacobrosenthal 23 bytes is the minimum MTU (and default). I'm reading from unofficial sources the maximum is 517 bytes. On Linux, bleno currently has a maximum MTU of 256 bytes, we can bump this up once we find the official maximum MTU size.
20 bytes is right for the maximum notification data size: 23 - 3
and 22 for the maximum size of read data if the MTU is 23: 23 -1
|
So I can get this to 'work' on osx by altering just like the 'connectable' demo, waiting until after advertising starts to setServices However its .. very odd. It works once (ie probably before some kind of caching) then if I dont disable ble and reenable, were getting a read before the MTU change first time after turning off ble or after a while (successful) second time (odd behavior) The mtuchange is in the onConnectionStateChange, Ive tried moving it forward to the onServicesDiscovered but had the same result.... |
|
Oh and one more thing, removing the mtu change request in android 'fixes' this with obviously many more transfers |
Cleanup and fixes for osx
|
I was traveling and fell behind on my github responsibilities. Where does this stand. Is this ready to merge or are there still some strong issues that still need to be resolved? |
|
@jacobrosenthal can fill us in, but I don't think this can be merged. From what I recall OS X + Android had some weird caching issues with the MTU exchange. I've proposed an alternative idea for the protocol instead of using read attributes here: google/physical-web#784 (comment) |
5 participants
Based on the discussion in google/physical-web#814 this should add Fat Beacon support to node-eddystone-beacon