Skip to content

getUnixEpoch() method added #16

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

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Conversation

UT2UH
Copy link
Contributor

@UT2UH UT2UH commented Mar 30, 2021

Unix Epoch time returned as uint32_t to cast to user provided time library 'time_t' type.

@PaulZC
Copy link
Collaborator

PaulZC commented Mar 30, 2021

Hi @UT2UH ,
Thank you - this is much better. I will merge shortly. I just need to rename my Example24 first! ;-)
All the best,
Paul

@PaulZC PaulZC merged commit 691ea47 into sparkfun:release_candidate Mar 30, 2021
@adamgarbo
Copy link
Contributor

adamgarbo commented Mar 30, 2021

Great contribution @UT2UH! I've been meaning to add Unix epoch time functionality to the u-blox library for a while now.

Currently, the getUnixEpoch() function checks data.flags2.bits.confirmedTime to determine whether the time is valid. If this check fails, it returns a zero value. My thought is that the checks on the validity of the time and date should be done outside of the function, and it should simply calculate the unixtime based on whatever the values are in the registers. Personally, I'm often just as interested in what the incorrect value of the time is.

Additionally, were we to enforce these validity checks, I believe we'd also need to check data.flags2.bits.confirmedDate to ensure we have the correct date values from which to calculate the epoch time. Very commonly the confirmedTime flag is set first, but the confirmedDate takes longer to be asserted, during which the date can remain incorrect.

Cheers,
Adam

@adamgarbo
Copy link
Contributor

adamgarbo commented Mar 30, 2021

Another small suggestion is that if we'd like to have consistency across all SparkFun RTC libraries, the commonly used function name for reading epoch time is is .getEpoch().

@PaulZC
Copy link
Collaborator

PaulZC commented Mar 31, 2021

Hi @UT2UH ,

I must admit, I think it might be a good idea to remove the confirmedTime check from getUnixEpoch. That way users of modules with older firmware will be able to use the function. Not all modules /protocol versions support time validity. What are your thoughts please?

@adamgarbo: I'm not worried about the function name. get UnixEpoch makes it clear it is the 1970 Epoch, not any of the other variants. But thanks for the reminder!

Best wishes,
Paul

@UT2UH
Copy link
Contributor Author

UT2UH commented Mar 31, 2021

Hi there!
Thank you all for the responce. I am not yet too good with these validity bits so as I was interested only in valid time it looked like a good idea to check confirmedTime before returning microseconds or nanoseconds. My opinion - there should be overloaded function for the time rounded without nano/microseconds at all and another one for the valid time with user selected validity option. But I would leave the final decision to the community more experienced in GNSS time matters. Regarding the function name - as GPS has its own 'epoch' I would keep it clear.

@adamgarbo
Copy link
Contributor

Would it also be possible to modify .getUnixEpoch(); so as to make the microsecond argument optional? If I try to call the function without providing it with a variable to write to, I get the error:

/Users/adam/Documents/GitHub/Cryologger_Glacier_Velocity_Measurement_System/Software/cryologger_gvms_micromod_test/05_gnss.ino: In function 'void syncRtc()':
05_gnss:133:53: error: no matching function for call to 'SFE_UBLOX_GNSS::getUnixEpoch()'
         unsigned long gnssEpoch = gnss.getUnixEpoch(); // Get GNSS epoch time
                                                     ^
In file included from /Users/adam/Documents/GitHub/Cryologger_Glacier_Velocity_Measurement_System/Software/cryologger_gvms_micromod_test/cryologger_gvms_micromod_test.ino:20:
/Users/adam/Documents/Arduino/libraries/SparkFun_u-blox_GNSS_Arduino_Library/src/SparkFun_u-blox_GNSS_Arduino_Library.h:941:11: note: candidate: 'uint32_t SFE_UBLOX_GNSS::getUnixEpoch(uint32_t&, uint16_t)'
  uint32_t getUnixEpoch(uint32_t& microsecond, uint16_t maxWait = defaultMaxWait);
           ^~~~~~~~~~~~
/Users/adam/Documents/Arduino/libraries/SparkFun_u-blox_GNSS_Arduino_Library/src/SparkFun_u-blox_GNSS_Arduino_Library.h:941:11: note:   candidate expects 2 arguments, 0 provided

This would allow it to be called by users who aren't interested in microseconds.

Cheers,
Adam

@UT2UH
Copy link
Contributor Author

UT2UH commented Mar 31, 2021

Hi @adamgarbo,
I have added a new PR with overloaded function for rounded Unix epoch time without micros argument and have removed confirmedTime checking in the original function. May be it will be enough for all use cases.

@UT2UH UT2UH deleted the getTime branch March 31, 2021 14:50
@adamgarbo
Copy link
Contributor

Thanks, @UT2UH!

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.

3 participants