Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NRF.nfcRaw (and nfcUrl) limited to payload/message length 255 #2406

Closed
fanoush opened this issue Aug 24, 2023 · 11 comments · Fixed by #2407
Closed

NRF.nfcRaw (and nfcUrl) limited to payload/message length 255 #2406

fanoush opened this issue Aug 24, 2023 · 11 comments · Fixed by #2407

Comments

@fanoush
Copy link
Contributor

fanoush commented Aug 24, 2023

see also conversation https://forum.espruino.com/conversations/354213/#17099032

https://github.com/espruino/Espruino/blob/master/libs/bluetooth/jswrap_bluetooth.c#L2638
flatStrPtr is uint8_t * so the dataLen is truncated even if data buffer is allocated with full length here
https://github.com/espruino/Espruino/blob/master/libs/bluetooth/jswrap_bluetooth.c#L2626
and also terminated with full length here
https://github.com/espruino/Espruino/blob/master/libs/bluetooth/jswrap_bluetooth.c#L2641

same bug/feature is there for nfcUrl - two lines here truncate length to uint8_t
https://github.com/espruino/Espruino/blob/master/libs/bluetooth/jswrap_bluetooth.c#L2456-L2457

If this should not be supported then at least the length should be checked and rejected to not to allocate big buffers and terminate NDEF at wrong place.

@RonnyNewco
Copy link

The Pucket.js with the Nordic Semiconductor chip set supports up to 992 bytes. Limiting the usable size to 246 bytes should be avoided.

@fanoush
Copy link
Contributor Author

fanoush commented Aug 24, 2023

When checking the source and nfc tag 2 type an NDEF structures the header here https://github.com/espruino/Espruino/blob/master/libs/bluetooth/bluetooth.h#L319
is somewhat mixed bag regarding the length.

At position 0x10 there is 0x03 0x00 with comments TT - tag type (03), ML message len (0)
The ML byte is set from code as message length. So this allows only 1 byte.

And then there is TF = TNF and Flags = 0xC1. In this the SR bit in not set so the payload length then takes 4 bytes.

So the first part of the header does not allow size over 254 and then the NDEF payload is in long format to allow more, but only one byte is written in code anyway.

However If I understand it correctly the TT+ML is in fact something called TLV block described here
so the ML byte should be only up to value 254 since value 255 has special meaning of TLV_L_FORMAT_FLAG which says there are next two bytes having 16 bit length (?)

So my guess is to fix that

  • the payload is ok but we need to write long value (in big endian format?) there
  • the NFC 2 tag structure 0x03 0x00 could also be 0x03 0xff 0x00 0x00 and then the length could be 16 bit value (?)

@RonnyNewco
Copy link

RonnyNewco commented Aug 24, 2023

This s a dump from a tag I did successfully wrote to. Hopefully helpful on those questions...609 bytes payload..

Screenshot_20230824_130653_TagInfo

@fanoush
Copy link
Contributor Author

fanoush commented Aug 24, 2023

also found this https://stackoverflow.com/a/42108779 where it confirms my guess

The tag for this TLV is 0x03. The length can be either in one-byte format (for NDEF messages with a length between 0 and 254 bytes) or in three-byte format (for NDEF messages with a length of 255 or more bytes).

If you have longer NDEF message (e.g. one with 259 bytes), you would use the three-byte length format:
03 FF0103

So it should work, however I am not sure what happens when for simplicity we will encode length <255 in longer format i.e. 254 as 0x03 0xff 0x00 0xFE - should be valid but maybe it may confuse some implementations?

@RonnyNewco
Copy link

If it works with IOS/Android which can be tested my assumption is that it should be fine...

@fanoush
Copy link
Contributor Author

fanoush commented Aug 24, 2023

found NFCForum-TS-Type-2-Tag_1.1.pdf specification here https://github.com/somq/nfccard-tool/blob/master/docs/NFCForum-TS-Type-2-Tag_1.1.pdf and it says
image
so using longer format for smaller size is not as described there and may break some validation

@RonnyNewco
Copy link

Tried to work around that by right padding with space so the long format could allways be used but that hits browser specfic behaviour. Works with chrome but not with Safari/Firefox that sends this along and them som sites will crash. So this workaround is not viable...

@fanoush
Copy link
Contributor Author

fanoush commented Aug 25, 2023

@RonnyNewco if you want you can try PUCKJS build from https://github.com/espruino/Espruino/pull/2407/checks - download PUCKJS artifact here
image
it is dfu zip inside another zip

This code works for me in build for AMIIBOLINK NFC tag mentioned in the forum. I tried urls with ndef record length of 254,255 and up like this

>NRF.nfcURL("http://espruino.com?param="+'x'.repeat(254-22-5)+'z')
=undefined
>global['\xff'].NfcData
="_\xF2\xC8\xEDe\xE2\x86ed\3\xFF\xFF\xE1\x11|\x0F\3" ... "xxxxxxxxxxxxxxxz\xFE"
>NRF.nfcURL("http://espruino.com?param="+'x'.repeat(254-22-5-1)+'z')
=undefined
> 

my phone shows it just fine in NXP TagInfo app

@RonnyNewco
Copy link

RonnyNewco commented Aug 29, 2023

I am pretty much a novice in this domain. But when trying to update I get the following error message:

No manifest.json file found when selecting the zip file.

solved
This can be fixed by extracting and repackaging as the firmware loader assumes a flat structure and not a sub catalogue...

IMG_26F2501FD9E1-1

@fanoush
Copy link
Contributor Author

fanoush commented Aug 29, 2023

Yes, sorry, as mentioned it is zip inside zip, unpack the first one and use the one inside for DFU. Do not unpack the second one.
EDIT: oh you already solved it, great.

@RonnyNewco
Copy link

Perfect. Works as intended. Thank you very much for your effort. Much appropriated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants