NDEF_Buffer limitations and overflows

[maihde]

When I was working with larger NDEF records (WifiToken and VCard) I noticed limitations that were due to NDEF_Buffer having a maximum size of NDEF_MAX_SIZE which was set to 100 bytes. The initial issue was that IdentifyNDEF could be used to determine the NDEF_Type once a WifiToken or VCard was written to the tag. Upon further inspection, even writing a VCard was causing an overflow of NDEF_Buffer.

This PR addresses the limitations of NDEF_Buffer size without breaking backwards compatibility or behavior where the smaller NDEF buffer was expected or required. It also doesn't require the user to edit the library itself to increase the buffer size. This is accomplished by allowing begin to take an optional buffer to use instead of the default buffer. This gives the application control over the size of the buffer based on it's needs.

In addition, some new functions and refactoring was performed to simplify use of the class and make maintenance eaiser.

[fpistm]

Maybe just allow buffer length to be redefined would be enough ?

[maihde]

Hi @fpistm , I thought of that but decided on implementing an alternative approach for a few reasons:

• the use of the #define and the hidden buffer hides an important aspect of using the interface. Users need to read documentation to understand that they may need to change the constant to suit their needs and constraints. By making the buffer under the control of the user, it requires explicit acknowledgement about proper size of the buffer. If it weren't for the my desire to maintain 100% backwards compatibility I would have advocated for removing the default buffer entirely.

• using a user-provided buffer allows users the ability to choose compile-time or run-time allocation of memory.

• at least for my Arduino environment (v 2.1.1), it's not as simple as just dropping #define NDEF_MAX_SIZE (1024) at the top of the sketch. This is because the library files get compile as individual compilation units, so the redefined constant would have no effect. I'd like to avoid having the user to edit the library source directly and Ardunio makes it complicated (IMHO) to adjust to library compile environment by the user.

[fpistm]

• at least for my Arduino environment (v 2.1.1), it's not as simple as just dropping #define NDEF_MAX_SIZE (1024) at the top of the sketch. This is because the library files get compile as individual compilation units, so the redefined constant would have no effect. I'd like to avoid having the user to edit the library source directly and Ardunio makes it complicated (IMHO) to adjust to library compile environment by the user.

That is normal as sketch fil is converted as ino.cpp.
To redefine properly a define you have to use build_opt.h
So, to redefine it here
https://github.com/stm32duino/ST25DV/blob/3e22326ec701ddbf64de98705adb1017be44651f/src/libNDEF/lib_NDEF.h#L56C1-L57

code should be :

+ #ifndef NDEF_MAX_SIZE               
 #define NDEF_MAX_SIZE               (100)
+ #endif
+ #ifndef NDEF_RECORD_MAX_SIZE        
 #define NDEF_RECORD_MAX_SIZE        (100)
+ #endif

Then thanks the build_opt.h you can simply set:
-DNDEF_MAX_SIZE=256

[cparata]

Hello @fpistm ,
build_opt.h redefinition works also with other platforms? Maybe we have some users that use this library with Arduino boards or ESP chipsets. I don't know if these platforms support also the build_opt.h feature.
Best Regards,
Carlo

[fpistm]

atforms? Maybe we have some users that use this library with Arduino boards or ESP chipsets. I don't know if these platforms support also the build_opt.h feature.
Best Regards,

Right. I know ESP can do this as they contact me to know how it was done on our side and implements something similar.
Anyway, I guess you are right it is not the correct solution.

One drawback I can see with this PR is the default buffer is always allocated even if it is not used.
Maybe malloc could be used to allocate it at begin en eventually reallocate if request data are higher.

[maihde]

@cparata from my research, I agree with you. The ability to use build_opt.h is tied to the Ardunio board support package and not Ardunio itself. For example, the STDM32 package supports it but the SAMD (used by MKR1000) does not appear to. I'll keep trying, but at least with my environment I haven't found a way to easily modify library build flags.

That said, even for environments that support build_opt.h my personal preference is an explicit buffer, since it avoids hidden behavior and makes it clear to the user that they need to think about their use case and how big of a buffer to allocate.

@fpistm I kept the default buffer for API backwards compatibility, if you are willing to break APIs that I would personally choose to remove the default buffer. I didn't use malloc for the fallback case because 100 bytes seemed small enough to not bother most people and I don't have broad enough experience with all Arduino environments to understand negative complications that might result from silently converting the behavior to dynamic memory. I was concerned that even through the API didn't change it might cause subtle behavior differences to users.

[cparata]

Hello all,
@fpistm, I agree with you that it is not an optimized solution in terms of memory allocation. It is also true that we have the waste of memory only if we want to use a different size buffer from the default one, so in the normal usage we do not have any issue. Probably malloc/realloc solution is more efficient but it requires also the usage of heap memory that maybe is not ideal for some platforms. The solution proposed by @maihde seems to me a good compromise, so I would integrate it.
Best Regards,
Carlo

[cparata]

Hello @maihde ,
also in this PR we have AStyle issues. Anyway if you fix them in PR #30, it should be fine and we can close this PR.
Best Regards,
Carlo