Add firmware_update support to c8y mapper

[rina23q]

Proposed changes

Implement firmware_update support to c8y-mapper as described in #2453.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#2453

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Merging #2464 (18a3a1a) into main (5259a45) will increase coverage by 0.0%.
The diff coverage is 82.4%.

Additional details and impacted files

Files	Coverage Δ
crates/core/tedge_api/src/mqtt_topics.rs	85.7% <100.0%> (+<0.1%)	⬆️
...extensions/c8y_mapper_ext/src/config_operations.rs	86.8% <ø> (+0.2%)	⬆️
crates/extensions/c8y_mapper_ext/src/lib.rs	88.8% <100.0%> (+1.3%)	⬆️
crates/extensions/c8y_mapper_ext/src/log_upload.rs	88.0% <ø> (+0.3%)	⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	80.6% <75.0%> (-0.1%)	⬇️
crates/extensions/c8y_mapper_ext/src/converter.rs	81.4% <78.5%> (+<0.1%)	⬆️
crates/extensions/c8y_mapper_ext/src/config.rs	43.1% <0.0%> (-1.8%)	⬇️
crates/core/tedge_api/src/messages.rs	90.3% <0.0%> (-3.2%)	⬇️
...s/extensions/c8y_mapper_ext/src/firmware_update.rs	86.4% <86.4%> (ø)

... and 1 file with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
379	0	3	379	100	55m42.633999999s

[albinsuresh]

Another field that I'm not sure if tedge should support, just because C8y supports it. What's the use-case I wonder. Why/when would/should a device report the URL with which the firmware was installed? Is this mandatory to send this url field after finishing the firmware update with a C8y operation?

More of a question to @reubenmiller I guess.

[reubenmiller]

Generally if the url of the firmware binary is known then it should be included. If you are talking about publishing the current firmware info (name, version) on startup, then it is possible that the url is not known (as it might be baked into the OS image and was not provided via a url).

[albinsuresh]

Since the url is useless in the initial bootstrap message, that reports the factory firmware info on the device, the only other case where it'd be useful is after a firmware update is performed. Is the use-case something like this: A device gets its firmware updated with some binary in the URL. The device is not capable of detecting the name/version even after the update. So, all that it can report back to the cloud is that it is using the firmware that it received in the URL?

Even in this case I don't think this is needed, because the successful status message already includes that remoteUrl field, which the mapper is using to send the 115 message. So, including the same url here seems redundant/unnecessary.

[reubenmiller]

I don't have the context anymore of what message we're talking about. But more information can be useful..but without concrete examples of what messages we're explicitly talking about then I can't make an informed decision.

[reubenmiller]

But having the url property can't harm the message, and it is more descriptive about where the firmware came from (if the info is available).

[albinsuresh]

I don't have the context anymore of what message we're talking about. But more information can be useful..but without concrete examples of what messages we're explicitly talking about then I can't make an informed decision.

Okay, so here are the two cases where this "tedge firmware metadata" message would be sent:

1. When a device is getting bootstrapped: it needs to publish its factory firmware details. In this case, at best the device would know its version and name. So, the device is less likely to use the url field in this path.
2. After the device firmware is updated using a cloud request (with a remote URL): the firmware_update request would contain that remoteUrl and once this operation is successful, the device would send the successful command status message with the same remoteUrl. The mapper on receipt of this successful status will mark the forward that to the cloud (SR 503 message), followed by the SmartREST set firmware message (SR 115) where the same remoteUrl in the successful status response is used. Since the firmware version in the cloud is already updated this way, the device need not send the tedge firmware metadata message again. Now, on the next restart of the agent, this tedge firmware metadata message would be sent again, but here also, it won't be able to send that url information, unless it persists that URL info after every firmware update, which is less likely/unnecessary IMO.

This is the reason why I feel that the url field in the "tedge firmware metadata message" is completely useless and it is relevant only in the firmware_update command messages.

But having the url property can't harm the message, and it is more descriptive about where the firmware came from (if the info is available).

Yeah, having an extra field doesn't harm, but when we document this API, we'll have to give some guidance to the user on when/how to use it, right? If this field is never going to be used, as neither cases highlighted above had a need for it (without additional persistence requirements), why even have it as an optional field?

[reubenmiller]

I think we should still keep it as it "could" be useful to people if they want to implement it (e.g. workflow overriding etc.).

[albinsuresh]

I'd have taken the opposite approach of omitting it for now and adding it when we encounter the need for it, as it's always easy to add things to a public API than removing things. Even for the workflow overriding, I only see the remoteUrl field in the firmware update cmd message being used and not this url field in the capability message.

But, since you're fairly confident that it would be useful in some use-cases, and since it's an optional field anyway, let's keep it for now. Once firmware update is fully implemented, and if this field is still not used, we can still remove it before 1.0.

[albinsuresh]

The code changes LGTM. Only two outstanding comments to be discussed/resolved:

1. The need for url field in the firmware metadata message
2. Moving the cpability enabled check and warning into the process_smartrest function.

[rina23q]

2. Moving the cpability enabled check and warning into the process_smartrest function.

I removed the unreachable code by bfd52a3. However, removed the warnings since it was difficult to move warnings as there are possibilities that custom operation may catch the disabled SmartREST ID.

[albinsuresh]

LGTM