Update sfe_sara_r5.h

[PaulZC]

Add the #define's from SARA_R5_ to UBLOX_AT_

This allows existing SARA_R5 code to compile and run, fixing errors like:

With these changes, I think SparkFun_u-blox_SARA-R5_Arduino_Library.h is no longer required and can be deleted.

We need to have a separate discussion about how we should handle (e.g.) the supported baud rates. We need a way for each derived class to be able to override the supported baud rates. I believe the code is currently defaulting to 115200, which is OK and works on the SARA. But SARA needs to support slower bauds too:

We also need to have a discussion about how to handle this code in processURCEvent. This code is specific to the SARA_R5 and so shouldn't be in the UBLOX_AT class. We need a way to allow the SARA_R5 class to 'insert' this. Maybe we need to add a virtual method which does nothing in UBLOX_AT, but which can be overwritten by the SARA_R5 class to allow UUPSDA to be processed?

Same for this line in pruneBacklog.

[sfe-SparkFro]

I think I'm going to vote no on this PR, but happy to hear other opinions! @gigapod?

The point of this library is to minimize the amount of duplicate code between different modules. That means one single set of macros/constants/enums/etc. between all modules; take the AT command strings for example:

https://github.com/sparkfun/SparkFun_u-blox_AT_Commands_Arduino_Library/blob/37db6a8333ade98c89385b8078f5a00d126e9064/src/sfe_ublox_at_commands.h#L95-L104

Merging this would mean we have duplicate AT command string definitions, but with different macro names:

https://github.com/sparkfun/SparkFun_u-blox_AT_Commands_Arduino_Library/blob/37db6a8333ade98c89385b8078f5a00d126e9064/src/sfe_sara_r5.h#L28-L37

I'm guessing you're suggesting this change for backwards compatibility with the original SARA-R5 library, which is the exclusive purpose of SparkFun_u-blox_SARA-R5_Arduino_Library.h. The idea is that if a user does not want to update their code to the new macros and such, they can use the original #include and SARA_R5_ macros. But in my mind, those are effectively deprecated in this library and should not be used.

As for your other comments regarding baud rates and URC processing - lets move those discussions to separate issues so we can keep each thread on topic.

Edit: See #2 and #3

[PaulZC]

I'm mostly concerned about backward compatibility. Right now, this does not compile:

if (mySARA.readMQTT(&qos, &topic, buf, MQTT_MAX_MSG_SIZE, &len) == SARA_R5_SUCCESS)

Including SparkFun_u-blox_SARA-R5_Arduino_Library.h doesn't solve this because readMQTT now returns UBLOX_AT_error_t and SARA_R5_SUCCESS is SARA_R5_error_t.

Having #define SARA_R5_ERROR_SUCCESS UBLOX_AT_ERROR_SUCCESS and #define SARA_R5_SUCCESS SARA_R5_ERROR_SUCCESS in sfe_sara_r5.h solves the issue.

I don't mind if these defines are in SparkFun_u-blox_SARA-R5_Arduino_Library.h or sfe_sara_r5.h, but I'm sure they need to be somewhere?

I'm less worried about all the const char AT command string declarations. But, because those are public in the SARA_R5 library, users may be using them in their own derived methods or classes. So I vote including them even though they are duplicates. Those can stay in SparkFun_u-blox_SARA-R5_Arduino_Library.h if needed.

If there's a better way to solve this, let's hear it! ;-)

[sfe-SparkFro]

because readMQTT now returns UBLOX_AT_error_t and SARA_R5_SUCCESS is SARA_R5_error_t

Oh! Yep, you're right I hadn't caught that. In that case, I think it would be best to put these changes into SparkFun_u-blox_SARA-R5_Arduino_Library.h, and leave that as the one file that exists for backwards compatibility. I can do this real quick!

[sfe-SparkFro]

@PaulZC Ok, how's this? All changes are now in SparkFun_u-blox_SARA-R5_Arduino_Library.h. If this all works, I'm happy to merge it in!

[PaulZC]

That looks good - thanks!

Unless of course you're just about to change UBLOX_AT_ to something else?!

Enjoy the holiday,
Paul

[sfe-SparkFro]

you're just about to change UBLOX_AT_

Doh! Yes, will update that then merge.

Thank you!

[gigapod]

I think I'm going to vote no on this PR, but happy to hear other opinions! @gigapod?

The point of this library is to minimize the amount of duplicate code between different modules. That means one single set of macros/constants/enums/etc. between all modules; take the AT command strings for example:

https://github.com/sparkfun/SparkFun_u-blox_AT_Commands_Arduino_Library/blob/37db6a8333ade98c89385b8078f5a00d126e9064/src/sfe_ublox_at_commands.h#L95-L104

Merging this would mean we have duplicate AT command string definitions, but with different macro names:

https://github.com/sparkfun/SparkFun_u-blox_AT_Commands_Arduino_Library/blob/37db6a8333ade98c89385b8078f5a00d126e9064/src/sfe_sara_r5.h#L28-L37

I'm guessing you're suggesting this change for backwards compatibility with the original SARA-R5 library, which is the exclusive purpose of SparkFun_u-blox_SARA-R5_Arduino_Library.h. The idea is that if a user does not want to update their code to the new macros and such, they can use the original #include and SARA_R5_ macros. But in my mind, those are effectively deprecated in this library and should not be used.

As for your other comments regarding baud rates and URC processing - lets move those discussions to separate issues so we can keep each thread on topic.

Edit: See #2 and #3

FYI - When it comes to code / libraries - things people depend on - backward compatibility is critical. We don't want to break someones code. If it takes a little extra layer code to do this, it's a-okay. Just document the need for this in the code ..

And for this macro redefinition - while a devtime hit, runtime costs are 0.

For future implementations - we should use a single macro / enum/symbol strategy.