samples: bluetooth: fast_pair: input_device: Improve nRF54H20 memory map #19174

alstrzebonski · 2024-11-29T17:07:34Z

Puts Fast Pair provisioning partition right before the storage
partition.
Aligns partition sizes and addresses to be a multiple of 0x10, which is
the write block size for MRAM.
Keeps the storage partition at address divisible by 0x1000, as this is
the storage sector size.
Fixes the cpuapp_rw_partitions node deletion (earlier it was done
incorrectly).

Jira: NCSDK-30269

NordicBuilder · 2024-11-29T17:08:53Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 12

Inputs:

Sources:

sdk-nrf: PR head: 9b49a91695a74d56ac89a9fe02059ff95520c376

more details

sdk-nrf:

PR head: 9b49a91695a74d56ac89a9fe02059ff95520c376
merge base: 729cb583e9dd237f4500705b5b7a51f41e825f73
target head (main): 729cb583e9dd237f4500705b5b7a51f41e825f73
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (1)

samples
│  ├── bluetooth
│  │  ├── fast_pair
│  │  │  ├── input_device
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay

Outputs:

Toolchain

Version:
Build docker image:

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain
◻️ Build twister
◻️ Integration tests
- ◻️ test-fw-nrfconnect-ble_samples

Disabled integration tests

- desktop52_verification
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

mkapala-nordic · 2024-12-02T09:07:44Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

-			label = "bt_fast_pair";
-			reg = <0x1e8fb8 0x48>;
+		storage_partition: partition@1e3080 {
+			reg = <0x1e3080 0x5f80>;


please check how the storage subsys will behave when there will be such partition that is not divisable by the 0x1000 (i think it was default sector size)

So, it says that I have five sectors of 4096 bytes which is 0x5000 in hex. The extra 0xf80 is unused and should be removed I guess. However, if I set the reg to: reg = <0x1e3080 0x5000>;, the sample fails to boot. It only works if I set it to reg = <0x1e4000 0x5000>; (or reg = <0x1e3080 0x5f80>; as it's done now, but then we're wasting the extra 0xf80). @rghaddab, could you expand more on that? Anyway, it seems that we should start from the address divisible by 0x1000. I will set the storage partition to reg = <0x1e4000 0x5000>; and the Fast Pair partition to reg = <0x1e3f80 0x80>; so that it precedes the storage partition. We will leave the gap between the DFU partition and Fast Pair partition, but I think it's the best we can do.

alstrzebonski · 2024-12-02T15:14:39Z

Spotted that the write block size is actually equal to 16 (0x10) and thus aligned all sizes and addresses to be divisible by 0x10. Also moved the storage partition to start at address divisible by 0x1000 as this is the storage sector size.

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

mkapala-nordic · 2024-12-02T15:35:33Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

+		/* Leaving here an empty gap from 0x1e3000 to 0x1e3fb0.
+		 * We want to have a bt_fast_pair_partition preceding the
+		 * storage_partition. We also need a storage partition to
+		 * start at address divisible by 0x1000, because this is the
+		 * storage sector size. What is more, we want to have all
+		 * partition sizes and addresses divisible by 0x10 which is
+		 * the write block size for MRAM.
+		 */


Consider: TBH I personaly would leave only the information that there is the gap between this and that address.

I do not think that it is requirement to have bt_fast_pair_partition before the settings partition and it is only our "style" i guess to be aligned with other apps, right?
And the rest is just how the storage / MRAM works.

If needed we could add it in commit message i guess :D

I think it has educational value to have it here and explain what is happening.

Maybe we could set bt_fast_pair_partition size to 0x1000 (to avoid having unassigned memory). That would simplify configuration a bit.

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

MarekPieta · 2024-12-03T08:32:35Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

+		/* Leaving here an empty gap from 0x1e3000 to 0x1e3fb0.
+		 * We want to have a bt_fast_pair_partition preceding the
+		 * storage_partition. We also need a storage partition to
+		 * start at address divisible by 0x1000, because this is the
+		 * storage sector size. What is more, we want to have all
+		 * partition sizes and addresses divisible by 0x10 which is
+		 * the write block size for MRAM.
+		 */


Maybe we could set bt_fast_pair_partition size to 0x1000 (to avoid having unassigned memory). That would simplify configuration a bit.

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

alstrzebonski · 2024-12-03T14:04:36Z

Rebased on main to fix compliance.

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

rghaddab · 2024-12-04T10:10:22Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

-			label = "bt_fast_pair";
-			reg = <0x1e8fb8 0x48>;
+		/* The storage partition needs to start at address divisible
+		 * by 4096 (0x1000), because this is the storage sector size.


Actually the storage sector size is not a fix value and can be set by the app, I don't know where you got the information that the storage sector size should be 4096 bytes for the nrf54h20.
For NVS this size should be a multiple of the erase-block-size value that is set in the device tree
For ZMS this size should be a multiple of the write-block-size (I think it is 16 bytes for the nrf54h20).
I am not arguing about the alignment here, as this is necessary for NVS, but the comment is kind of misleading

I know it can be set by application, but in our sample it's 4096 so that's what I'm referring to. Also, from what I see the alignment is also needed for ZMS - when I set the storage partition to reg = <0x1e3080 0x5000>;, the sample fails to boot. I guess it would be nice to have some build assert for asserting that the storage partition starts at address divisible by storage sector size (from what I experience this is the silent requirement)

ZMS do not need the alignment of the start address to the sector size. It should be aligned only to the write-block-size
If the sample fails to boot, then there is a bug that needs to be reported.

So in that case there's bug here. I set start address to 0x1e3080 so it was aligned to write-block-size which is 16 bytes (0x10), but it failed to boot. What is more when I increased the size (reg = <0x1e3080 0x5000>; -> reg = <0x1e3080 0x5f80>;) it started working.

So @rghaddab, what do you want the comment to look like?

MarekPieta · 2024-12-05T07:45:02Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

 		#address-cells = <1>;
 		#size-cells = <1>;

 		dfu_partition: partition@100000 {
 			reg = <0x100000 DT_SIZE_K(908)>;
 		};

-		storage_partition: partition@1e3000 {
-			reg = <0x1e3000 DT_SIZE_K(20)>;
+		/* Align the partition size to 4096 B to avoid gaps. */


Please also update the commit message

MarekPieta · 2024-12-05T07:46:06Z

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay

-		bt_fast_pair_partition: partition@1e8fb8 {
-			label = "bt_fast_pair";
-			reg = <0x1e8fb8 0x48>;
+		/* The storage partition needs to start at address divisible


This comment might need to be aligned with @rghaddab to ensure correctness

@alstrzebonski I don't think you need here to justify why the address starts at 0x1e4000 as this is where the previous partition stops.

The justification is misleading. Currently the storage system doesn't require an alignment of the start address to the sector size. There might be a bug there that needs to be fixed later.

I don't agree. There's a bug here so currently the storage system does require an alignment. If the bug is fixed then it will no longer require it, but for now it does.

I can rephrase it to:

/* Due to a bug, the storage partition needs to start at address divisible * by 4096 (0x1000), because this is the storage sector size * in this sample. */

Do you approve?

Much simpler : remove the comment in the device tree, but keep it in the commit message.
Why do you need to justify in the device tree that the address starts at 4K boundary if the previous one stops there ?

Previous one stops there to not leave gaps.

Ok, I will remove the comment. Will you take over the bug or do I need to open issue in Zephyr?

@alstrzebonski I will take over the bug

rghaddab · 2024-12-06T10:02:35Z

@alstrzebonski in the commit message you can write up to 75 chars in a line.
No need for a new line if this doesn't exceed the 75 limit.
Here is a suggestion:
Puts fast pair provisioning partition right before the storage partition
and sets its size to 4 KiB to avoid gaps.
Align the storage partition to a 4KiB boundary as this is the sector size.
This alignment is needed due to a bug in the storage subsystem.
Fixes the cpuapp_rw_partitions node deletion (earlier it was done
incorrectly).

alstrzebonski · 2024-12-06T13:05:23Z

Puts fast pair provisioning partition right before the storage partition
and sets its size to 4 KiB to avoid gaps.
Align the storage partition to a 4KiB boundary as this is the sector size.
This alignment is needed due to a bug in the storage subsystem.
Fixes the cpuapp_rw_partitions node deletion (earlier it was done
incorrectly).

Addressed, @rghaddab please rereview.

rghaddab · 2024-12-06T15:49:43Z

Puts fast pair provisioning partition right before the storage partition
and sets its size to 4 KiB to avoid gaps.
Align the storage partition to a 4KiB boundary as this is the sector size.
This alignment is needed due to a bug in the storage subsystem.
Fixes the cpuapp_rw_partitions node deletion (earlier it was done
incorrectly).

Addressed, @rghaddab please rereview.

Sorry @alstrzebonski for insisting on the form of the commit message, but this is still not Ok.
Maybe my last message wasn't clear. The 75 char limit must be respected but you have to make a new line if this is a new sentence.
In my suggestion I put a new line after each "." point mark

alstrzebonski · 2024-12-09T10:23:03Z

Puts fast pair provisioning partition right before the storage partition
and sets its size to 4 KiB to avoid gaps.
Align the storage partition to a 4KiB boundary as this is the sector size.
This alignment is needed due to a bug in the storage subsystem.
Fixes the cpuapp_rw_partitions node deletion (earlier it was done
incorrectly).

Addressed, @rghaddab please rereview.

Sorry @alstrzebonski for insisting on the form of the commit message, but this is still not Ok. Maybe my last message wasn't clear. The 75 char limit must be respected but you have to make a new line if this is a new sentence. In my suggestion I put a new line after each "." point mark

Never before heard about this new line after each sentence. Also, the limit is 72 char. Anyway, I addressed it. Please rereview.

rghaddab · 2024-12-09T10:27:26Z

Never before heard about this new line after each sentence. Also, the limit is 72 char.

https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-message-guidelines

alstrzebonski · 2024-12-09T10:30:47Z

Never before heard about this new line after each sentence. Also, the limit is 72 char.

https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-message-guidelines

Nothing about new line after each sentence.

Puts fast pair provisioning partition right before the storage partition and sets its size to 4 KiB to avoid gaps. Align the storage partition to a 4KiB boundary as this is the sector size. This alignment is needed due to a bug in the storage subsystem. Fixes the cpuapp_rw_partitions node deletion (earlier it was done incorrectly). Jira: NCSDK-30269 Signed-off-by: Aleksander Strzebonski <[email protected]>

rghaddab · 2024-12-09T10:35:10Z

Nothing about new line after each sentence.

makes sense when all the sentences are related to the same thing.
But when you are fixing 3 different things, a new line is better.
Like I said that's not a blocker, just a preference

rghaddab

Looks Ok now

alstrzebonski · 2024-12-09T10:38:40Z

Nothing about new line after each sentence.

makes sense when all the sentences are related to the same thing. But when you are fixing 3 different things, a new line is better. Like I said that's not a blocker, just a preference

you have to make a new line if this is a new sentence. - doesn't look as just a preference. Anyway, please just give approve and let's not waste any more time on that.

alstrzebonski requested a review from a team as a code owner November 29, 2024 17:07

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 29, 2024

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from dce371e to c3508e4 Compare November 29, 2024 17:18

mkapala-nordic reviewed Dec 2, 2024

View reviewed changes

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from c3508e4 to 6096480 Compare December 2, 2024 15:12

alstrzebonski requested a review from mkapala-nordic December 2, 2024 15:12

mkapala-nordic reviewed Dec 2, 2024

View reviewed changes

MarekPieta requested changes Dec 3, 2024

View reviewed changes

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from 6096480 to 2755e2f Compare December 3, 2024 13:59

alstrzebonski requested review from mkapala-nordic and MarekPieta December 3, 2024 14:00

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from 2755e2f to 4649cfc Compare December 3, 2024 14:04

mkapala-nordic approved these changes Dec 3, 2024

View reviewed changes

MarekPieta requested changes Dec 3, 2024

View reviewed changes

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay Outdated Show resolved Hide resolved

samples/bluetooth/fast_pair/input_device/boards/nrf54h20dk_nrf54h20_cpuapp.overlay Show resolved Hide resolved

rghaddab reviewed Dec 4, 2024

View reviewed changes

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from 4649cfc to 29d3c41 Compare December 4, 2024 14:15

alstrzebonski requested a review from MarekPieta December 4, 2024 14:16

alstrzebonski added this to the 2.9.0 milestone Dec 4, 2024

shanthanordic modified the milestones: 2.9.0, 2.9.0-nRF54H20 Dec 4, 2024

MarekPieta approved these changes Dec 5, 2024

View reviewed changes

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch 2 times, most recently from 98ddf1c to 92d2460 Compare December 6, 2024 09:48

alstrzebonski requested a review from rghaddab December 6, 2024 09:49

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from 92d2460 to d6a9a46 Compare December 6, 2024 13:05

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from d6a9a46 to cff0d41 Compare December 6, 2024 13:09

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from cff0d41 to dc6ca06 Compare December 9, 2024 10:26

alstrzebonski force-pushed the fp_lilium_improve_memory_map2 branch from dc6ca06 to 9b49a91 Compare December 9, 2024 10:32

rghaddab approved these changes Dec 9, 2024

View reviewed changes

jukkar merged commit bce1276 into nrfconnect:main Dec 9, 2024
12 checks passed

alstrzebonski removed this from the 2.9.0-nRF54H20 milestone Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: bluetooth: fast_pair: input_device: Improve nRF54H20 memory map #19174

samples: bluetooth: fast_pair: input_device: Improve nRF54H20 memory map #19174

alstrzebonski commented Nov 29, 2024 •

edited

Loading

NordicBuilder commented Nov 29, 2024 •

edited

Loading

mkapala-nordic Dec 2, 2024

alstrzebonski Dec 2, 2024 •

edited

Loading

alstrzebonski commented Dec 2, 2024

mkapala-nordic Dec 2, 2024

alstrzebonski Dec 2, 2024

MarekPieta Dec 3, 2024

MarekPieta Dec 3, 2024

alstrzebonski commented Dec 3, 2024

rghaddab Dec 4, 2024

alstrzebonski Dec 4, 2024 •

edited

Loading

rghaddab Dec 4, 2024

alstrzebonski Dec 4, 2024 •

edited

Loading

alstrzebonski Dec 6, 2024

MarekPieta Dec 5, 2024

MarekPieta Dec 5, 2024

rghaddab Dec 6, 2024

alstrzebonski Dec 6, 2024

alstrzebonski Dec 6, 2024

rghaddab Dec 6, 2024

alstrzebonski Dec 6, 2024

alstrzebonski Dec 6, 2024

rghaddab Dec 6, 2024

rghaddab commented Dec 6, 2024

alstrzebonski commented Dec 6, 2024

rghaddab commented Dec 6, 2024

alstrzebonski commented Dec 9, 2024 •

edited

Loading

rghaddab commented Dec 9, 2024

alstrzebonski commented Dec 9, 2024

rghaddab commented Dec 9, 2024

rghaddab left a comment

alstrzebonski commented Dec 9, 2024 •

edited

Loading

samples: bluetooth: fast_pair: input_device: Improve nRF54H20 memory map #19174

samples: bluetooth: fast_pair: input_device: Improve nRF54H20 memory map #19174

Conversation

alstrzebonski commented Nov 29, 2024 • edited Loading

NordicBuilder commented Nov 29, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Choose a reason for hiding this comment

alstrzebonski Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

alstrzebonski commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstrzebonski commented Dec 3, 2024

Choose a reason for hiding this comment

alstrzebonski Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstrzebonski Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rghaddab commented Dec 6, 2024

alstrzebonski commented Dec 6, 2024

rghaddab commented Dec 6, 2024

alstrzebonski commented Dec 9, 2024 • edited Loading

rghaddab commented Dec 9, 2024

alstrzebonski commented Dec 9, 2024

rghaddab commented Dec 9, 2024

rghaddab left a comment

Choose a reason for hiding this comment

alstrzebonski commented Dec 9, 2024 • edited Loading

alstrzebonski commented Nov 29, 2024 •

edited

Loading

NordicBuilder commented Nov 29, 2024 •

edited

Loading

alstrzebonski Dec 2, 2024 •

edited

Loading

alstrzebonski Dec 4, 2024 •

edited

Loading

alstrzebonski Dec 4, 2024 •

edited

Loading

alstrzebonski commented Dec 9, 2024 •

edited

Loading

alstrzebonski commented Dec 9, 2024 •

edited

Loading