applications: connectivity_bridge: thingy91 debug #18146

maxd-nordic · 2024-10-23T22:37:22Z

Add the CMSIS-DAP feature of the thingy91x to the thingy91.

maxd-nordic · 2024-10-23T22:37:48Z

applications/connectivity_bridge/boards/thingy91_nrf52840.conf

+CONFIG_CMSIS_DAP_DEVICE_VENDOR="Nordic Semiconductor ASA"
+CONFIG_CMSIS_DAP_DEVICE_NAME="nrf91"
+
+CONFIG_HEAP_MEM_POOL_SIZE=6000


RAM is tight - had to decrease usage.

FYI: A lot of RAM is used for the USB CDC FIFOs. In prj.conf, CONFIG_USB_CDC_ACM_RINGBUF_SIZE=16384. As there are 2 x CDC instances, there are 4 of these buffers in total (=64 KiB RAM). So only a slight reduction of this parameter could give you the RAM you need,
Sadly the "old" USB CDC class doesn't let you set the buffer size on a per instance basis. The USB-next version will. I suspect only the CDC instance used for trace would need this kind of buffering.

alright, I slightly reduced the cdc buffer instead

I hope the missing 1K isn't going to cause any trouble with certificate provisioning. There still is a lot of buffer.

NordicBuilder · 2024-10-23T22:40:11Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 0e377ad3822eed59b332429b40dcb47d4ec7636f

more details

sdk-nrf:

PR head: 0e377ad3822eed59b332429b40dcb47d4ec7636f
merge base: 83118d7d268baf9fb13c5260023fcd9b6e3aa4b6
target head (main): 423981109ef39b942e73e2b8e91c64692bc4d566
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 (6)

applications
│  ├── connectivity_bridge
│  │  ├── boards
│  │  │  ├── thingy91_nrf52840.conf
│  │  │  ├── thingy91_nrf52840_readme.txt
│  │  │  │ thingy91x_nrf5340_cpuapp.conf
│  │  ├── src
│  │  │  ├── modules
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  │ usb_bulk_interface.c

Outputs:

Toolchain

Version: f51bdba1d9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:f51bdba1d9_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 42
✅ Integration tests

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_cloud
- 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

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

NordicBuilder · 2024-10-23T22:54:33Z

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.

applications/connectivity_bridge/src/modules/CMakeLists.txt

maxd-nordic · 2024-11-05T14:03:57Z

This depends on zephyrproject-rtos/zephyr#80920. Gotta fix an error that I introduced myself. 🙈

NordicBuilder · 2024-11-08T09:37:35Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
zephyr	nrfconnect/sdk-zephyr@`54adfa7` (main)	nrfconnect/sdk-zephyr#2245	nrfconnect/sdk-zephyr#2245/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

gmarull · 2024-11-08T09:41:49Z

applications/connectivity_bridge/src/modules/usb_bulk_interface.c

+#if defined(CONFIG_BOARD_THINGY91X)
 	len = dap_execute_vendor_cmd(buf->data, tx_buf);
+#else
+	len = dap_execute_cmd(buf->data, tx_buf);
+#endif /* defined(CONFIG_BOARD_THINGY91X) */


in general, adding board dependencies in code is a bad practice. The clean way to to this is to revert the dependency, ie

An app level Kconfig (or none)

Boards that want DAP simply enable DAP, or select app level option

Application enables DAP based on DAP Kconfigs or the custom one if required.

I understand the notion. In this case, the application is tailored to only two specific boards and we needed some place for this code to go. These features are very specific to these boards, but also don't really have a place in board files.

@gmarull is the above response OK?

What if this application is used by a customer with its own custom board? Board specific code in applications is never a good idea. Never.

This doesn't add board specific code. It reuses code specific to the Thingy:91 X.

This is not a sample. If customers want to use it with other boards, they have to port it. Other applications work just the same.

Again: board switches at application level are not acceptable. This makes application non-easily portable and makes life unnecessary difficult for end users. The right thing to do this is:

Add an application level setting, like CONFIG_APP_ENABLE_THIS

Make sure board overlay enables it if needed

This way, app stays board agnostic, and if any customer porting this app needs the option, they can just touch a conf overlay and not C code.

Also, consider the case where a user creates a board with the same features as Thingy:91 because they want to run this application (so HW requirements are covered), but maybe they just change e.g. the PMIC or adjust its form factor. Then, instead of being able to use the sample as is with some overlays, they'll be forced to essentially create a copy and patch the sample code.

nordicjm

Awaiting confirmation from @gmarull

nordicjm · 2024-11-14T07:17:14Z

applications/connectivity_bridge/src/modules/CMakeLists.txt

+		     app PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/usb_bulk_interface.c)
+
+target_sources_ifdef(CONFIG_BOARD_THINGY91X_NRF5340_CPUAPP
+			app PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/usb_bulk_commands.c)


align properly as with above lines

nordicjm · 2024-11-14T07:18:04Z

applications/connectivity_bridge/src/modules/usb_bulk_interface.c

+#if defined(CONFIG_BOARD_THINGY91X)
 	len = dap_execute_vendor_cmd(buf->data, tx_buf);
+#else
+	len = dap_execute_cmd(buf->data, tx_buf);
+#endif /* defined(CONFIG_BOARD_THINGY91X) */


@gmarull is the above response OK?

gmarull

lgtm

Add the CMSIS-DAP feature of the thingy91x to the thingy91. Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic requested review from a team and nordic-auko as code owners October 23, 2024 22:37

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

maxd-nordic commented Oct 23, 2024

View reviewed changes

maxd-nordic force-pushed the t91-debug-feature branch from 0f84499 to fbb1bc1 Compare October 23, 2024 22:38

maxd-nordic requested review from jtguggedal and simensrostad October 23, 2024 22:41

nordicjm reviewed Oct 24, 2024

View reviewed changes

applications/connectivity_bridge/src/modules/CMakeLists.txt Outdated Show resolved Hide resolved

maxd-nordic force-pushed the t91-debug-feature branch 2 times, most recently from e321ef2 to 8a60a3e Compare October 24, 2024 15:16

bjda approved these changes Oct 24, 2024

View reviewed changes

simensrostad approved these changes Oct 24, 2024

View reviewed changes

maxd-nordic force-pushed the t91-debug-feature branch from 8a60a3e to 920283a Compare November 8, 2024 09:37

maxd-nordic requested a review from a team as a code owner November 8, 2024 09:37

github-actions bot added the manifest label Nov 8, 2024

NordicBuilder added manifest-zephyr DNM labels Nov 8, 2024

maxd-nordic force-pushed the t91-debug-feature branch from 920283a to e14a2b2 Compare November 8, 2024 09:38

gmarull reviewed Nov 8, 2024

View reviewed changes

maxd-nordic force-pushed the t91-debug-feature branch from e14a2b2 to f7fd87d Compare November 13, 2024 15:03

github-actions bot removed the manifest label Nov 13, 2024

maxd-nordic added backport v2.8-branch auto-create a PR with same commits to v2.8-branch and removed DNM manifest-zephyr changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Nov 13, 2024

nordicjm requested changes Nov 14, 2024

View reviewed changes

maxd-nordic force-pushed the t91-debug-feature branch from f7fd87d to 3c183e6 Compare November 14, 2024 09:35

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

maxd-nordic force-pushed the t91-debug-feature branch from 3c183e6 to 5a0b3a9 Compare November 14, 2024 11:24

gmarull reviewed Nov 14, 2024

View reviewed changes

applications: connectivity_bridge: thingy91 debug

0e377ad

Add the CMSIS-DAP feature of the thingy91x to the thingy91. Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic force-pushed the t91-debug-feature branch from 5a0b3a9 to 0e377ad Compare November 14, 2024 11:35

maxd-nordic requested a review from nordicjm November 14, 2024 12:12

nordicjm approved these changes Nov 14, 2024

View reviewed changes

nordicjm merged commit 9598724 into nrfconnect:main Nov 14, 2024
13 checks passed

NordicBuilder mentioned this pull request Nov 14, 2024

[Backport v2.8-branch] applications: connectivity_bridge: thingy91 debug #18895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applications: connectivity_bridge: thingy91 debug #18146

applications: connectivity_bridge: thingy91 debug #18146

maxd-nordic commented Oct 23, 2024

maxd-nordic Oct 23, 2024

nordic-auko Oct 24, 2024

maxd-nordic Oct 24, 2024

maxd-nordic Oct 24, 2024

NordicBuilder commented Oct 23, 2024 •

edited

Loading

NordicBuilder commented Oct 23, 2024

maxd-nordic commented Nov 5, 2024

NordicBuilder commented Nov 8, 2024 •

edited

Loading

gmarull Nov 8, 2024

maxd-nordic Nov 13, 2024

nordicjm Nov 14, 2024

gmarull Nov 14, 2024

maxd-nordic Nov 14, 2024

maxd-nordic Nov 14, 2024

gmarull Nov 14, 2024

gmarull Nov 14, 2024

nordicjm left a comment

nordicjm Nov 14, 2024

nordicjm Nov 14, 2024

gmarull left a comment

applications: connectivity_bridge: thingy91 debug #18146

applications: connectivity_bridge: thingy91 debug #18146

Conversation

maxd-nordic commented Oct 23, 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

NordicBuilder commented Oct 23, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

NordicBuilder commented Oct 23, 2024

maxd-nordic commented Nov 5, 2024

NordicBuilder commented Nov 8, 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

nordicjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull left a comment

Choose a reason for hiding this comment

NordicBuilder commented Oct 23, 2024 •

edited

Loading

NordicBuilder commented Nov 8, 2024 •

edited

Loading