Skip to content
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

cache_create: Extended the command to extract from envelope #155

Merged

Conversation

ahasztag
Copy link
Collaborator

This commit extends the suit-generator cache_create command.

It splits it into two commands: from_payloads and from_envelope.

from_payload has the same functionality as the old version of the cache_create command

from_envelope allows extracting payloads directly from the envelope

  • also with the possibility to parse the envelope hierarchically and omit certain payloads.

suit_generator/cmd_cache_create.py Outdated Show resolved Hide resolved
suit_generator/cmd_cache_create.py Outdated Show resolved Hide resolved
suit_generator/cmd_cache_create.py Outdated Show resolved Hide resolved
@ahasztag ahasztag force-pushed the NCSDK-28932_hierarchical_cache_create branch 2 times, most recently from d138c4b to 05af7e5 Compare November 22, 2024 09:01
args = single_input.split(",")
if len(args) < 2:
raise ValueError("Invalid number of input arguments: " + single_input)
uri, input_file = args
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, consider validation of uri's duplication.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added such validation

raise ValueError("Number of required padding bytes exceeds assumed max size 0xFFFF")
cmd_cache_create_from_envelope.add_argument(
"--dependency-regex",
help="Integrated payloads matching the regular expression will be treated as dependency envelopes and parsed hierarchically."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing space will result in following output:

  --dependency-regex DEPENDENCY_REGEX
                        Integrated payloads matching the regular expression will be treated as dependency envelopes and parsed hierarchically.The
                        payloads extracted from the dependency envelopes will be added to the cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

else:
raise GeneratorError("The provided envelope/dependency envelope is not a valid envelope!")

if dependency_regex is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since --dependency-regex is optional argument user can execute script and ecounter error due to uninitialized integrated_dependencies

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, probably this is better now

:param omit_payload_regex: Integrated payloads matching the regular expression will not be extracted to the cache
:param dependency_regex: Integrated payloads matching the regular expression will be treated as dependency envelopes
"""
envelope = cbor2.loads(envelope_data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data which is not a valid suit envelope can potentialy crash during cbor decoding or (if decoded as non taged structure) during access to envelope.value in line 224.

Example for nordic_top envelope with --dependency-regex ".*sys.*":

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x90 in position 0: invalid start byte

Example for nordic_top envelope with --dependency-regex ".*sec.*":

AttributeError: 'int' object has no attribute 'value'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified error logging so that this results in a hopefully clearer error

Copy link
Collaborator

@kszromek-nordic kszromek-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider adding protection against uris duplication in case of cache creation from envelope, payloads and merging.

Add a cache slot to the cache from the given URI and data.

This function creates a cache slot from the given URI and data, and pads the data to align with the specified
erase block size (eb_size). The first slot in the cache is created with indefinite length CBOR map.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure here but what about last slot - should it include break byte so whole binary length is multiplicity of eb_size (and not bigger by this one break byte)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed - the end of map marker 0xFF must be placed in a new erase block.

If the device itself would try to add a new cache slot it would otherwise face a problem - the current end of map marker would have to be removed. This would cause the need to erase the whole previous erase block with data from the previous slot.
Though we might need to waste one erase block (at the end of the cache), this is unavoidable.

@ahasztag ahasztag force-pushed the NCSDK-28932_hierarchical_cache_create branch from 05af7e5 to 92e013c Compare November 28, 2024 13:55
@NordicBuilder
Copy link
Collaborator

NordicBuilder commented Nov 28, 2024

pytest coverage results

Detailed report:

Type Coverage
lines 91.3% (2136 of 2340 lines)
functions no data found
branches no data found

Note: This message is automatically posted and updated by the CI (latest/test-sdk-dfu/master/248)

@ahasztag ahasztag force-pushed the NCSDK-28932_hierarchical_cache_create branch 3 times, most recently from 38639bb to 4841671 Compare November 29, 2024 12:47
@ahasztag ahasztag force-pushed the NCSDK-28932_hierarchical_cache_create branch 2 times, most recently from 50da469 to 4342a03 Compare December 2, 2024 07:18
This commit extends the suit-generator cache_create command.

It now has three subcommands: from_payloads, from_envelopei and merge.

from_payload has the same functionality as the old version of
the cache_create command

from_envelope allows extracting payloads directly from the envelope
- also with the possibility to parse the envelope hierarchically
and omit certain payloads.

merge allows to merge multiple cache partition files into one.

Ref: NCSDK-28932

Signed-off-by: Artur Hadasz <[email protected]>
@ahasztag ahasztag force-pushed the NCSDK-28932_hierarchical_cache_create branch from 4342a03 to 145f904 Compare December 2, 2024 08:11
@ahasztag ahasztag merged commit 0e66051 into nrfconnect:ncs Dec 2, 2024
2 checks passed
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.

5 participants