Boards:shield:added nrf5340 audio display #11909

philipfidjeland · 2023-07-31T08:49:34Z

No description provided.

NordicBuilder · 2023-07-31T08:49:52Z

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

CLAassistant · 2023-07-31T09:18:36Z

All committers have signed the CLA.

andvib

First read through, focused on structure changes

applications/nrf5340_audio/CMakeLists.txt

applications/nrf5340_audio/prj.conf

applications/nrf5340_audio/src/main.c

applications/nrf5340_audio/src/modules/CMakeLists.txt

applications/nrf5340_audio/src/modules/display/button_tab.c

applications/nrf5340_audio/src/modules/display/display.c

applications/nrf5340_audio/src/modules/display/log_tab.c

applications/nrf5340_audio/tools/buildprog/nrf5340_audio_dk_devices.json

applications/nrf5340_audio/src/modules/hw_codec.h

boards/shields/nrf5340_audio_display/Kconfig.shield

koffes · 2023-07-31T12:11:15Z

applications/nrf5340_audio/CMakeLists.txt

@@ -8,6 +8,9 @@ cmake_minimum_required(VERSION 3.20.0)

 file(TO_CMAKE_PATH $ENV{ZEPHYR_BASE} CMAKE_STYLE_ZEPHYR)

+
+set(SHIELD nrf5340_audio_display)


Should this be added even if the display is not present?

I guess 'if (NRF5340_AUDIO_DK_DISPLAY)' can be used here

if NRF5340_AUDIO_DK_DISPLAY, in cmake is set to late if the config is not set as a west build flag. So i have just baked the shield and kconfig in a "--display" flag in buildprog.py

The build strategy is now that the user must set both the shield and NRF5340_AUDIO_DK_DISPLAY

koffes · 2023-07-31T12:13:55Z

applications/nrf5340_audio/src/main.c

@@ -25,6 +25,7 @@
 #include "audio_system.h"
 #include "channel_assignment.h"
 #include "streamctrl.h"
+#include <display/display.h>


Display is a very generic name. Let me read through the PR and see if I have another suggestion.

koffes · 2023-07-31T12:14:44Z

applications/nrf5340_audio/src/main.c

@@ -168,6 +169,11 @@ int main(void)
 {
 	int ret;

+	if (IS_ENABLED(CONFIG_DISPLAY)) {
+		ret = display_init();


If the display is configured as a driver: Can the display_init() be run there instead of main?

koffes · 2023-07-31T12:16:37Z

applications/nrf5340_audio/src/modules/Kconfig

+
+menu "Display"
+
+config DISPLAY


I would move all the display related KConfigs into the display folder.

koffes · 2023-07-31T12:17:12Z

applications/nrf5340_audio/src/modules/Kconfig

+
+config DISPLAY
+	bool "True if using adafruit display"
+	default n


If the project is compiled with the display shield, this should be set automatically.

The build strategy is now that the user must set both the shield and NRF5340_AUDIO_DK_DISPLAY

koffes · 2023-07-31T13:08:00Z

applications/nrf5340_audio/src/modules/hw_codec.c

@@ -32,6 +32,10 @@ static cs47l63_t cs47l63_driver;

 /**@brief Write to multiple registers in CS47L63
 */
+uint32_t hw_codec_volume_get()
+{
+	return prev_volume_reg_val;


I need to look into this, but the only way to "properly" get the volume is to read it from the HW codec itself. Discuss, and see if we have another option using VCS.

I guess this value will also be available on zbus when the "mothership" PR is merged?

koffes · 2023-07-31T13:09:27Z

boards/shields/nrf5340_audio_display/nrf5340_audio_display.overlay

+
+    ili9340: ili9340@2 {
+        compatible = "ilitek,ili9340";
+        spi-max-frequency = <15151515>;


This value looks strange.

Iknow, But it is the only value that seems to work?

Correction, done some more testing and it works for values under 17000000 (unless they are very small)

Made this value 150000. This is the highest round number the screen seems to tolerate

koffes · 2023-07-31T13:10:02Z

boards/shields/nrf5340_audio_display/nrf5340_audio_display.overlay

+        ngamctrl = [ 00 0e 14 03 11 07 31 c1 48 08 0f 0c 31 36 0f  ];
+    };
+
+    adafruit_2_8_tft_touch_v2_sdhc: sdhc@3 {


It is likely best to just disable the SD slot here all together.

koffes · 2023-07-31T13:10:42Z

boards/shields/nrf5340_audio_display/nrf5340_audio_display.overlay

@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2019 PHYTEC Messtechnik GmbH


Do we need all of this for the overlay? It should be enough to define some pins, SPI speed etc.

Most of the numbers for the screen are a bit magic, even after reading the documentation. They're copied directly from the Zephyr generic overlay, and changing some of them causes the screen to not work, and others just changes the color scheme. Philip did testing for all of them and concluded they were necessary.

koffes · 2023-07-31T13:13:24Z

lib/pcm_stream_channel_modifier/pcm_stream_channel_modifier.c

@@ -10,8 +10,10 @@
 #include <errno.h>

 #include <zephyr/logging/log.h>
+#include <stdlib.h>


There should IMO be no changes to this file. We need to look for another place to put this functionality. This is a NCS global file and should only do the exact thing it says in the doc an nothing more. Unit tests are also required for changes here.

lib/pcm_stream_channel_modifier/pcm_stream_channel_modifier.c

applications/nrf5340_audio/src/modules/display/button_tab.c

applications/nrf5340_audio/src/modules/display/log_tab.c

applications/nrf5340_audio/src/modules/hw_codec.h

include/pcm_stream_channel_modifier.h

applications/nrf5340_audio/src/modules/display/log_tab.c

applications/nrf5340_audio/src/modules/display/Kconfig.defaults

applications/nrf5340_audio/src/modules/display/nrf5340_audio_dk_display.c

applications/nrf5340_audio/src/modules/display/button_tab.c

applications/nrf5340_audio/src/modules/display/tab_menu.c

erikrobstad

I have not looked closely at the logic in the .c files yet, and I have not looked at the board files

erikrobstad · 2023-08-15T12:35:38Z

applications/nrf5340_audio/CMakeLists.txt

@@ -8,6 +8,9 @@ cmake_minimum_required(VERSION 3.20.0)

 file(TO_CMAKE_PATH $ENV{ZEPHYR_BASE} CMAKE_STYLE_ZEPHYR)

+
+set(SHIELD nrf5340_audio_display)


I guess 'if (NRF5340_AUDIO_DK_DISPLAY)' can be used here

erikrobstad · 2023-08-15T12:37:15Z

applications/nrf5340_audio/Kconfig

@@ -40,6 +40,7 @@ rsource "src/drivers/Kconfig"
 rsource "src/modules/Kconfig"
 rsource "src/utils/Kconfig"
 rsource "dfu/conf/Kconfig.dfu"
+rsource "src/modules/display/Kconfig"


This can be removed, since it is added in src/modules/Kconfig

erikrobstad · 2023-08-15T12:38:31Z

applications/nrf5340_audio/src/modules/CMakeLists.txt

@@ -14,3 +14,17 @@ target_sources(app PRIVATE
 	       ${CMAKE_CURRENT_SOURCE_DIR}/power_meas.c
 	       ${CMAKE_CURRENT_SOURCE_DIR}/sd_card.c
 )
+
+if (CONFIG_AUDIO_DFU_ENABLE)


This PR should not add any DFU related code

Dont think it does, Just a whitespace that has sneaked in...

erikrobstad · 2023-08-15T12:39:45Z

applications/nrf5340_audio/src/modules/CMakeLists.txt

+
+if (CONFIG_DISPLAY)
+	target_sources(app PRIVATE
+		${CMAKE_CURRENT_SOURCE_DIR}/display/nrf5340_audio_dk_display.c


I think it is cleaner to add a CMakeLists.txt in the display folder

I agree, is now implemented!

erikrobstad · 2023-08-15T12:40:41Z

applications/nrf5340_audio/src/modules/Kconfig

@@ -5,13 +5,13 @@
 #

 rsource "Kconfig.defaults"
-
+rsource "display/Kconfig"


Add back the newline

erikrobstad · 2023-08-15T13:28:05Z

applications/nrf5340_audio/src/modules/display/tab_menu.h

+ */
+void tab_menu_create(lv_obj_t *current_screen);
+/**
+ * @brief Updates all objects on the button tab


Suggested change

* @brief Updates all objects on the button tab

* @brief Update all objects on the button tab

erikrobstad · 2023-08-15T13:28:48Z

applications/nrf5340_audio/src/modules/hw_codec.c

@@ -123,6 +123,7 @@ static void volume_msg_sub_thread(void)
 /**
 * @brief Write to multiple registers in CS47L63.
 */
+


No changes to this file should be done in this PR?

I will leave this up to Andreas. This might now be possible to get as a zbus message?

Replied to this on the comment in the header file

erikrobstad · 2023-08-15T13:29:10Z

applications/nrf5340_audio/src/modules/hw_codec.h

@@ -21,6 +21,16 @@
 */
 int hw_codec_volume_set(uint8_t set_val);

+/**


No changes to this file should be done in this PR?

This has been discussed around the office, and while this value is probably at this point, it has been agreed that a get function here is a fair addition to the hw_coded module

erikrobstad · 2023-08-15T13:29:26Z

applications/nrf5340_audio/tools/buildprog/nrf5340_audio_dk_devices.json

@@ -1,16 +1,16 @@
 [
 {
-        "nrf5340_audio_dk_snr": 1000,
+        "nrf5340_audio_dk_snr": 1050133518,


Remove the changes in this file

erikrobstad · 2023-08-15T13:29:42Z

boards/shields/nrf5340_audio_display/Kconfig.defconfig

+# Copyright (c) 2023 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+#


Newline after license

Adds the nrf5340_audio_dk_display module for the audio application. This module uses the LVGL library to present information from the application to the user using an Adafruit touch display. A shield definition is added so that the Adafruit display shield can be used with the nrf5340 Audio DK. Signed-off-by: Philip Fidjeland <philipfidjeland@gmail.com> Signed-off-by: Andreas Vibeto <andreas.vibeto@nordicsemi.no>

github-actions · 2023-11-15T00:27:30Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

koffes · 2023-11-21T09:43:10Z

I have picked some of this code into other branches. At present time we need to focus on other areas, so as it stands, I will close this PR for now.

philipfidjeland requested review from koffes, alexsven, erikrobstad, rick1082, gWacey, anangl, rlubos and tejlmand as code owners July 31, 2023 08:49

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jul 31, 2023

NordicBuilder added the external External contribution label Jul 31, 2023

andvib added the DNM label Jul 31, 2023

andvib self-requested a review July 31, 2023 08:59

andvib removed request for tejlmand, anangl and rlubos July 31, 2023 09:36

andvib added the CI-Requested Approves single commit for CI tests on Internal HW label Jul 31, 2023

andvib removed request for koffes, alexsven, erikrobstad, rick1082 and gWacey July 31, 2023 11:46

andvib suggested changes Jul 31, 2023

View reviewed changes

koffes self-requested a review July 31, 2023 12:07

andvib added CI-Requested Approves single commit for CI tests on Internal HW and removed CI-Requested Approves single commit for CI tests on Internal HW labels Jul 31, 2023

koffes requested changes Jul 31, 2023

View reviewed changes

philipfidjeland commented Jul 31, 2023

View reviewed changes

lib/pcm_stream_channel_modifier/pcm_stream_channel_modifier.c Outdated Show resolved Hide resolved

github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Aug 1, 2023

philipfidjeland requested review from andvib and koffes August 8, 2023 10:47

koffes requested review from rick1082, alexsven and erikrobstad August 9, 2023 07:42

andvib suggested changes Aug 9, 2023

View reviewed changes

andvib suggested changes Aug 15, 2023

View reviewed changes

philipfidjeland force-pushed the log_tab branch 2 times, most recently from f6d0223 to 7948499 Compare August 15, 2023 10:18

andvib removed the external External contribution label Aug 15, 2023

erikrobstad suggested changes Aug 15, 2023

View reviewed changes

andvib force-pushed the log_tab branch from 1112d90 to 81716ba Compare August 17, 2023 13:17

andvib force-pushed the log_tab branch from 81716ba to 08510f9 Compare August 28, 2023 10:05

andvib force-pushed the log_tab branch from 08510f9 to 5902f51 Compare September 15, 2023 13:48

andvib requested review from andvib and removed request for andvib September 15, 2023 13:50

andvib removed the DNM label Sep 15, 2023

andvib force-pushed the log_tab branch from 5902f51 to ddb0b18 Compare September 15, 2023 14:02

andvib requested review from erikrobstad and andvib and removed request for andvib September 15, 2023 14:02

andvib approved these changes Sep 15, 2023

View reviewed changes

andvib requested review from andvib and removed request for andvib September 15, 2023 14:04

github-actions bot added the Stale label Nov 15, 2023

koffes closed this Nov 21, 2023

		@@ -8,6 +8,9 @@ cmake_minimum_required(VERSION 3.20.0)

		file(TO_CMAKE_PATH $ENV{ZEPHYR_BASE} CMAKE_STYLE_ZEPHYR)


		set(SHIELD nrf5340_audio_display)

		@@ -0,0 +1,64 @@
		/*
		* Copyright (c) 2019 PHYTEC Messtechnik GmbH

	* @brief Updates all objects on the button tab
	* @brief Update all objects on the button tab

Boards:shield:added nrf5340 audio display #11909

Boards:shield:added nrf5340 audio display #11909

Conversation

philipfidjeland commented Jul 31, 2023

NordicBuilder commented Jul 31, 2023

CLAassistant commented Jul 31, 2023 • edited Loading

andvib left a 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

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

erikrobstad left a 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

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

github-actions bot commented Nov 15, 2023

koffes commented Nov 21, 2023

CLAassistant commented Jul 31, 2023 •

edited

Loading