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

fix: support using ReadURI with SMS and GEO type #30

Merged
merged 9 commits into from
Jul 31, 2023

Conversation

maihde
Copy link
Contributor

@maihde maihde commented Jul 22, 2023

If you write a GEO or SMS URI with the WriteURI function, for example:

st25dv.writeURI("", "sms:+17035946155", "")

It will fail when using ReadURI because NDEF_ParseURI has special logic for looking at the URI string and changing the NDEF_Type. For symmetry in the API and to make ReadURI more versatile I have added support to read the SMS and GEO types with ReadURI.

@fpistm fpistm requested a review from cparata July 22, 2023 17:56
This allows IdentifyNDEF to work when the NDEF is larger than the
default NDEF_MAX_SIZE
The default buffer of 100 bytes is too small for Wifi and VCard.
Further, using the default buffer prevents a user from picking a buffer
size more suited to their processor environment and tag needs.  This
change retains backwards API compatibility but allows a user to provider
their own buffer as part of begin()
@cparata
Copy link
Contributor

cparata commented Jul 24, 2023

Hi @maihde ,
first of all, thanks for the contribution! I just have few comments. Why did you not use the APIs available in the NDEF library? There are dedicated APIs for SMS and GEO with helper functions in the files lib_NDEF_SMS.cpp/h and lib_NDEF_Geo.cpp/h. I would reuse those APIs to extend the features of the library. What do you think about it?
Best Regards,
Carlo

@maihde
Copy link
Contributor Author

maihde commented Jul 24, 2023

@cparata yes, I agree that the NDEF library APIs are available. This PR is primarily about ensuring symmetry with the API, it seemed to be unintentional that writeURI can write an abridged URI and then an immediate call to readURI would fail. I also think it's more intuitive to use readURI for these simple cases and prevents users from having to write code to handle various cases where they may not know what type of URI is on the tag. Finally, it also seemed pretty low-risk since no new APIs were introduced and it seemed unlikely that someone would have software relying on readURI to failing in this situation.

@cparata
Copy link
Contributor

cparata commented Jul 25, 2023

Hello @maihde ,
I would prefer to add dedicated high level APIs to support other data formats besides writeURI/readURI . In my opinion, the correct path should be using the low level APIs that are already available in the NDEF library in the files lib_NDEF_SMS.cpp/h and lib_NDEF_Geo.cpp/h to implement new high level APIs like writeSMS/readSMS and writeGeo/readGeo. Unfortunately, I do not have band to work on it, but it would be great if the community gives his contribution to improve the library.
Best Regards,
Carlo

@maihde
Copy link
Contributor Author

maihde commented Jul 25, 2023 via email

@cparata
Copy link
Contributor

cparata commented Jul 26, 2023

Hello @maihde ,
in my opinion, you are trying to use writeURI and readURI in a way that they were not designed for. For SMS, Geo and Email there are dedicated low level APIs with specific structs. So, I prefer to see high level APIs based on the helper low level functions for the other specific cases.
Best Regards,
Carlo

@maihde
Copy link
Contributor Author

maihde commented Jul 26, 2023

Thanks @cparata! I don't wish to be a bother, so I'll assume the desire to have no change in behavior for writeURI and readURI unless I hear otherwise.

However, I wanted to provide a few more details since I may not be articulating the question well.

The existing behavior allows code like this.

// Write an abridged URI compatible with Apple URL background tag schemes
//https://developer.apple.com/documentation/corenfc/adding_support_for_background_tag_reading

// Abridged Prefix
st25dv.writeURI("mailto:", "[email protected]", "") // NDEF_OK
st25dv.readURI(&uri_read) // NDEF_OK

st25dv.writeURI("tel:", "+14085551212", "") // NDEF_OK
st25dv.readURI(&uri_read)  // NDEF_OK

// 'none' prefix
st25dv.writeURI("", "sms:+14085551212", "") // NDEF_OK
st25dv.readURI(&uri_read) // NDEF_ERROR

st25dv.writeURI("", "facetime://[email protected]", "") // NDEF_OK
st25dv.readURI(&uri_read) // NDEF_OK

st25dv.writeURI("", "X-HM://[email protected]", "") // NDEF_OK
st25dv.readURI(&uri_read) // NDEF_OK

// Not supported by iOS
st25dv.writeURI("", "geo:12345.123,12345.123", "") // NDEF_OK
st25dv.readURI(&uri_read) // NDEF_ERROR

How would you like writeURI to work?

  • Should writeURI succeed in all cases? (current behavior in baseline version)
  • Should writeURI return NDEF_ERROR for sms and geo?
  • Should writeURI return NDEF_ERROR for all arguments where the first argument is blank? (this would prevent writing custom URIs)

@cparata
Copy link
Contributor

cparata commented Jul 26, 2023

Hello @maihde ,
I think that using the writeURI without specifying any protocol is out of the scope of the writeURI and probably we could return an error when we use the API in this way.
Best Regards,
Carlo

Add the following features:

* The ability to read/write unabridged URIs per NDEF specifcation
* High-level API for SMS, GEO, and EMail
* Each API to read NDEF type
@maihde
Copy link
Contributor Author

maihde commented Jul 27, 2023

@cparata I have pushed an update that adds the following API to the ST25DV class:

  • writeUnabridgedURI/readUnabridgedURI - Support for NDEF protocol code 0x00 which means no URI prepending is performed. This allows users of the ST25DV to write URIs that are supported by Android/iOS devices but not part of the NDEF standard
  • write[SMS, GEO, EMail]/read[SMS, GEO, Email] - Support to read/write common URI types without having to use the NDEF class or NDEF structures
  • readNDEFType - When dealing with a tag where you don't know the type, this provides a simple high-level call for users that doesn't require them to use the NDEF object.

This code is based on fixes from #31 because the refactoring of NDEF_IdentifyNDEF makes the implementation of the above more concise. If you want to be able to pull these changes without also pulling #31 I can redo this PR.

@cparata
Copy link
Contributor

cparata commented Jul 27, 2023

Hello @maihde ,
thanks a lot for this contribution. We just need to fix AStyle and Codespell issues to pass CI check. Regarding Codespell, you can see the report here and the issue should be easily fixed. Regarding AStyle issues, you just need to run a script that fixes all code style issues as explained in this guide.
After these fixes, I can integrate the PR in the main branch.
Thanks again and Best Regards,
Carlo

@cparata cparata merged commit 4f6db07 into stm32duino:main Jul 31, 2023
3 checks passed
@cparata
Copy link
Contributor

cparata commented Jul 31, 2023

Hi @maihde ,
thanks a lot for your contribution. PR integrated.
Best Regards,
Carlo

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 this pull request may close these issues.

2 participants