Add get_time function to ESP8266

[michalpasztamobica]

Summary of changes

Fixes #12192.

get_time() function allows access to (S)NTP functionalities of ESP8266 module.

I considered using std::chrono instead of ctime's std::tm, but then - to convert to chrono::time_point we'd have to go through std::tm anyway or write our own parser, which I just managed to avoid.

Side note: using std:: introduces some warnings due to -D_LIBCPP_EXTERN_TEMPLATE(...)= flag being used in all our compilation profiles. This flag seems obsolete and I hope I will get it removed soon.

I cannot really add any tests, as they are using WifiInterface::get_instance() to get the interface and there is no RTTI, that would allow casting to ESP8266Interface to call get_time(), but I tested the function locally and it works fine.

Impact of changes

None, other than user can check time with ESP8266Interface.

Migration actions required

None

Documentation

Added doxygen functions.

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

@AnttiKauppila
@VeijoPesonen
@SeppoTakalo
@kjbracey-arm
@star297

---

[michalpasztamobica]

I pushed astyle fixes.

[kjbracey]

I considered using std::chrono instead of ctime's std::tm

There will be a bunch of chrono changes coming soon, but at the minute I'm only considering the kernel millisecond clocks and the HAL ticker_api clocks, not the RTC/wall-clock time.

(I'm doing them as local KernelClock and TickerClock classes, similar to steady_clock and high_resolution_clock. No system_clock equivalent yet. They're going to be local classes because I don't think the retargetting across the 3 tool chains will allow nice consistent behaviour for the standard clocks).

So at the minute, I'd be inclined to stick with the traditional time_t or struct tm. If all the devices doing this give you something tm-like, then choosing that seems reasonable. Although we do use time_t for rtc_api.h.

write our own parser

Well, that's not necessary, you can use mktime to get to time_t, or _rtc_maketime from mbed_mktime.h, as the HALs do for their RTC.

[michalpasztamobica]

I think this PR doesn't need any work, does it?
@kjbracey-arm 's comment supports what I did: I am sticking to struct tm and I did not write my own parser. Even though there is no explicit approval I don't see any change requests either.
@adbridge , can you add "needs : CI" or "needs : review" tag?
@AnttiKauppila , could you review, please?

[star297]

We need a function returning 'seconds' rather than a 'ctime' format timestamp (unless I've missed something here).

[michalpasztamobica]

@star297 , I personally think it is better to return struct tm. It is closer to what the format that ESP chip is returning and it is trivial to convert it to the "seconds since epoch" format using a single mktime call (or to std::chrono as Kevin suggested above). Is this acceptable for you?

[star297]

Yes, from the 'device' output point, I agree that would better.

[AnttiKauppila]

Any reason why debug is now always disabled in ESP8266Interface.cpp? There is no mention of it in summary of changes

[michalpasztamobica]

Good point, @AnttiKauppila . I was just reluctant to modify the json file (which causes full rebuild of the code) and only modified the flags locally when checking if things work fine without logs. Then I forgot to remove this temporary solution.

It should be fine now.

[SeppoTakalo]

Can you please re-think your API.

I cannot find any relation between IEEE 802.11 WiFi networks and RFC 5905 Network Time protocol.

Even if some modules do support it, this should not bubble up to the public API as part of "network interface"
These two things have no relations between each others.

Original design is that NetworkInterface should be as abstract as possible, offering only connect(). And if the network requires, some extra functions that allow you to set it up. Like set_ssid().

If you insist that get_time() should be part of Mbed OS public API, similarly like DNS resolvers are, it should follow the same design principle and be part of NetworkStack instead of NetworkInterface. Then a default NTP resolver should be offered, when modules don't offer it. But it all cumulates Flash space, even if unused, because it will be part of stack's vtable.

[star297]

That is a very good point.
OS5 is a bit on the greedy side with resources.
How about adding these functions in the driver so the user can call them if they want to.
My ESP driver does just that so I can use low resource Targets.
But we definitely need a quick 'get time' function, the ESP version is good, I use mine all the time on OS5 baremetal and OS2.
Off topic, an Ethernet version would complement this nicely.

[michalpasztamobica]

I see your point, @SeppoTakalo . Indeed WiFiInterface's API might not be the most accurate place for get_time function.
If user creates an ESP8266Interface object by directly calling its constructor, rather than using the WiFiInterface::get_instance(), they will have access to the function (or if they allow RTTI).

@star297 , sounds like this is OK for you, as a reporter, so I'll amend the PR.

I will not be able to provide any tests with the PR, though, I hope this is fine?

Pull request has been modified.

[michalpasztamobica]

This is now ready for another review, @SeppoTakalo , @AnttiKauppila

[michalpasztamobica]

I am investigating the current failures. For greentea tests - I doubt this is related to my changes, the platform which failed was NUCLEO_F411RE. I tried to rerun, but the job is stuck.
For the dynamic-memory-usage - it failed on wifi.bin test with K66F that indeed has ESP8266. I am now checking locally if it is indeed this commit that broke things.

[michalpasztamobica]

I could reproduce the dynamic-memory-usage failure locally. I will investigate this and hopefully push a fix soon.
The F411RE failure in greentea test has repeated after rerunning, so it is most likely also something that needs work from my side.

Pull request has been modified.

[michalpasztamobica]

It seems that the ESP8266 chips which are attached to NUCLEO_F411RE's do not have the update firmware (no V1.7 tag) and the SNTP command was only added in v1.5. No SW fix can be made for this and we'd better support older firmware by default.
For K66F - I do not know the root cause, to be honest, the tag is in place, but sending the command causes the device to hang for some reason. Perhaps the tag is false? I don't have access to hardware now in order to verify it.

As this is a very optional and experimental feature I just wrapped its usage with a macro, so that it only runs if enabled. This fixed the two failures observed in CI and made the whole PR less intrusive for the code base.

Please, review the new changes and run CI again, to check that we're on the safe side.
Thanks!

[0xc0170]

CI started meanwhile

[star297]

It seems that the ESP8266 chips which are attached to NUCLEO_F411RE's do not have the update firmware (no V1.7 tag) and the SNTP command was only added in v1.5. No SW fix can be made for this and we'd better support older firmware by default.

You really shouldn't support ESP8266 below firmware v1.7
Just depreciate it and if necessary drop any relevant platforms, technology and Mbed moves forward.

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 5
Build artifacts

[SeppoTakalo]

@michalpasztamobica We already suggest updating to 1.7 firmware in https://github.com/ARMmbed/mbed-os/tree/master/components/wifi/esp8266-driver#firmware-version

Maybe if we just modify the document and suggest that we require at least 1.5 to work but support only 1.7 and newer?

[michalpasztamobica]

I did as you suggested, @SeppoTakalo .

[0xc0170]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 6
Build artifacts