-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
matter/fast pair: Add support for merging hex files without partition manager #18792
base: main
Are you sure you want to change the base?
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 5e5c4f7c6838f3324e863260de0b70850d263739 more detailssdk-nrf:
matter:
Github labels
List of changed files detected by CI (10)
Outputs:ToolchainVersion: f51bdba1d9 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
Memory footprint analysis revealed the following potential issuessample.matter.template.release[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 811902[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-18792/5) |
4df741a
to
2b3619b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
storage_partition: partition@1e3000 { | ||
reg = <0x1e3000 DT_SIZE_K(20)>; | ||
}; | ||
|
||
bt_fast_pair_partition: partition@1e8fb8 { | ||
label = "bt_fast_pair"; | ||
reg = <0x1e8fb8 0x48>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the addresses are correct. 20kB is equal to 0x5000 so I think the bt_fast_pair_partition
address should be set to 0x1e8000 (0x1e3000 + 0x5000) instead of 0x1e8fb8. Correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I positioned it at the end of the 0x1000 memory chunk to mimick the PM solution in the case of automatic builds without the specific memory layout file (pm.yaml). On the other hand, we typically position the provisioning data at the beginning of the 0x1000 memory chunk in builds where we define the memory layout file (pm.yaml).
@alstrzebonski, I don't have a preference here. We can align it according to your suggestion or we can leave this part and think about the position of the fast pair provisioning data hex in the nRF54H20 memory in the follow-up task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be aligned to my suggestion or, if we want to mimic the PM, we should add the comment explaining the gap in the memory map. I'm okay with both solutions. It can be also left as is and improved in follow-up task - maybe that's the best option considering it's @nordicjm's PR. It will be easier to leave it as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alstrzebonski, could you create a follow-up task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fast Pair part looks good to me. The configurations aspects of Fast Pair (e.g. partition position) can be improved in the follow-up PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of observations and a small proposal improvement, but nothing blocking.
set_property( | ||
GLOBAL APPEND PROPERTY SUIT_POST_BUILD_COMMANDS | ||
COMMAND ${PYTHON_EXECUTABLE} ${ZEPHYR_BASE}/scripts/build/mergehex.py | ||
--overlap replace | ||
-o ${OUTPUT_HEX_FILE} | ||
${ARTIFACTS_TO_MERGE} | ||
${ARTIFACTS_TO_MERGE} ${merge_files} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
observation: this is really not self-explaining what the difference between ARTIFACTS_TO_MERGE
and merge_files
is.
But as this file in general is messy, then there is not change requested for this PR.
Adds support for merging other files onto the uicr_merged.hex file Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Adds support for generating fast pair data for nRF54H20 which does not use partition manager Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no> Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Added nRF54H20 DK support to the Fast Pair Input Device sample. Ref: NCSDK-29602 Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
Updates matter to include support for generating factory data when partition manager is not used Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Calls the updated factory data function irrespective of partition manager enablement Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
2b3619b
to
5e5c4f7
Compare
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publish GitHub Action. |
For nRF54H20 only