gcoap/fileserver: add event callbacks

[benpicco]

Contribution description

When serving updates via GCoAP fileserver it turned out to be useful to know if a node started downloading the update and when it finished doing so.

But there are probably more use cases where some action might be taken or logged based on the events on the GCoAP fileserver.

This adds 5 event types:

• GCOAP_FILESERVER_GET_START: file download started (block 0 requested)
• GCOAP_FILESERVER_GET_END: file download finished (last block requested)
• GCOAP_FILESERVER_PUT_START:file upload started (block 0 uploaded)
• GCOAP_FILESERVER_PUT_END: file upload finished (last block uploaded)
• GCOAP_FILESERVER_DELETE:file deletion requested

Testing procedure

To be a able to test this, the new callback module is enabled for examples/gcoap_fileserver to log file events:

> ls /nvm0
./
../
LICENSE	24310 B
hello.txt	15 B
main.c	27 B
total 3 files
          
> gcoap fileserver: Download started: /nvm0/hello.txt
gcoap fileserver: Download finished: /nvm0/hello.txt

aiocoap-client coap://[fe80::748f:e6ff:fed7:426d%tapbr0]/vfs/hello.txt

Issues/PRs references

[chrysn]

Just to get the architectural review side out -- making sure more generally useful alternatives have been considered:

• This would hook into the specific interface between CoAP and the file service. Might it not make more sense to monitor things on the file system side, or on the CoAP side? (on the file system side it might behave like inotify, on the CoAP side like log hooks).

• What are use cases and possible extensions of this? (An extension I could envision is that at some point one might need to know who sent the request, and thus extend gcoap_fileserver_event_ctx_t, so maybe that should be private).

On use cases, note that CoAP GET is supposed to be side effect free; now we don't, can't and won't generally enforce this in RIOT, but reacting to a GET event in any other facility than logging (at which point I'd go back whether this might not be just for logging generic CoAP requests or file system access) sounds like an invitation to do side effects, and might need a warning.

[benpicco]

My use case is: I have multiple sensors that I want to update, since communication happens over a bus I want to update one sensor at a time to avoid contention. (The sensor network might also only be turned on for the updates when it's otherwise in an OFF state. So I want to know when all sensors have received the update to return to the original state again)

Updates are sometimes unreliable, especially with the old firmware revision. So I want to know: Was the manifest accepted (GCOAP_FILESERVER_GET_START on the ".bin" file) and finally: Did the update download successfully (GCOAP_FILESERVER_GET_END on the ".bin" file).

You are right that it would probably be better to use a second channel to communicate those events (e.g. the sensors would send them explicitly). But that won't help with sensors already deployed that don't send those yet to be implemented status messages - so server side events were an easy solution to this problem.

If more context information is required for other use cases, gcoap_fileserver_event_ctx_t can be extended without changing the API.

[fabian18]

The first thing I asked myself, was whether _event_cb and _event_ctx should be protected in some way by a mutex or atomic accesses because they are static variables and are read in the gcoap thread and can be written by any other thread using gcoap_fileserver_set_event_cb(), or if we just assume that they are set once, before requests are being processed.
But if they are set once, we could as well make _event_cb a classic __attribute__((weak)) hook and _event_ctx an extern variable, I guess. Any opinions on that?

[benpicco]

I realized that I want to unsubscribe from those events, so I'll go with the mutex.

[fabian18]

Works well with files.

2022-11-13 11:36:44,921 # gcoap fileserver: Upload started: /nvm0/send/ta/ca_anchor.der
2022-11-13 11:36:50,935 # gcoap fileserver: Upload finished: /nvm0/send/ta/ca_anchor.der
2022-11-13 11:36:50,977 # gcoap fileserver: Upload started: /nvm0/send/cp/self.der
2022-11-13 11:36:56,006 # gcoap fileserver: Upload finished: /nvm0/send/cp/self.der
2022-11-13 11:36:56,064 # gcoap fileserver: Upload started: /nvm0/send/key/pubkey.der
2022-11-13 11:36:57,292 # gcoap fileserver: Upload finished: /nvm0/send/key/pubkey.der
2022-11-13 11:36:57,348 # gcoap fileserver: Upload started: /nvm0/send/key/key.der
2022-11-13 11:36:58,986 # gcoap fileserver: Upload finished: /nvm0/send/key/key.der
2022-11-13 11:56:30,376 # gcoap fileserver: Download started: /nvm0/send/key/key.der
2022-11-13 11:56:30,476 # gcoap fileserver: Download finished: /nvm0/send/key/key.der
2022-11-13 11:57:14,149 # gcoap fileserver: Delete /nvm0/send/key/key.der
2022-11-13 11:59:07,298 # gcoap fileserver: Upload started: /nvm0/send/ta/ca_anchor.der
2022-11-13 11:59:13,227 # gcoap fileserver: Upload finished: /nvm0/send/ta/ca_anchor.der
2022-11-13 11:59:13,271 # gcoap fileserver: Upload started: /nvm0/send/cp/self.der
2022-11-13 11:59:18,292 # gcoap fileserver: Upload finished: /nvm0/send/cp/self.der
2022-11-13 11:59:18,348 # gcoap fileserver: Upload started: /nvm0/send/key/pubkey.der
2022-11-13 11:59:19,186 # gcoap fileserver: Upload finished: /nvm0/send/key/pubkey.der
2022-11-13 11:59:19,238 # gcoap fileserver: Upload started: /nvm0/send/key/.key.der
2022-11-13 11:59:20,934 # gcoap fileserver: Upload finished: /nvm0/send/key/.key.der

Someone might be wondering why it is only implemented for files?

[benpicco]

Someone might be wondering why it is only implemented for files?

I didn't see the need for it yet - should I add it anyway?

[fabian18]

If there is no need for directory callbacks right now, I´d say keep it like that.
To reflect that and for extensibility, you could maybe rename some fields, if you agree.
And maybe adapt some comments here and there.

[fabian18]

The msg_bus module just came to my mind. Have you considered this as an alternative to callbacks, since you already said that you would like to be able to "unsubscribe"? This may also be done in the future I guess.

[benpicco]

This could be implemented on top, but I'd like to avoid IPC if not necessary.

[fabian18]

Ok please squash.

[benpicco]

bors merge

[bors]

Build succeeded:

• Murdock
• static-tests
• tools-build-success

[benpicco]

Thank you for the review!