logger: disable sof-logger for zephyr #681

keqiaozhang · 2021-05-24T09:05:04Z

since zephyr doesnot support sof-logger, so add a function
to disable logger trace for zephyr during the test.

Signed-off-by: Zhang Keqiao keqiao.zhang@intel.com

marc-hb

Can you please add one more is_zephyr() function? I mean logger_disabled() invokes is_zephyr() like this:

is_zephyr()
{
  grep something.*zephyr somefile
}

logger_disabled()
{
   if is_zephyr; then
      return 0
    else ...

Can you please give an example of this /etc/sof/manifest.txt file and a link to what code creates and deploys it?

This /etc/sof/manifest.txt file may be good enough for CI in the very short term but I don't think it will work for most developers. I compile with and without Zephyr many times every day and I also use sof-test yet my /etc/sof/ has been empty since forever.

@andyross , @kv2019i , @lyakh : any other, simple, automatable and reliable way to tell the difference between plain SOF versus SOF+Zephyr?

PS: I've been using a .tarball-version hack that shows up in dmesg but this won't work for CI

case-lib/lib.sh

marc-hb · 2021-05-24T18:22:44Z

case-lib/lib.sh

+    manifest=/etc/sof/manifest.txt
+    FIRMWARE_TYPE=$(grep "firmwaretype" < "$manifest" | awk -F ':' '{print $2}' | sed -e 's/^[ ]*//g;s/\"//g')
+    # disable sof-logger for Zephyr built firmware
+    if [ "$FIRMWARE_TYPE" == "zephyr" ]; then


This above seems a bit long and over complicated, could this be shorten into something like this:

if grep `firmwaretype.*zephyr` /etc/sof/manifest.txt; then

Just a really simplified example: you may want a slightly more complex regular expression.

Good point, I will refine my patch.

I didn't know manifest.txt was actually a manifest.json. The standard, command-line parser for JSON is jq, please drop awk, grep etc. and use jq:

apt-get install jq jq '.version.firmwaretype' manifest.txt => "zephyr"

Updated. Thanks!

marc-hb · 2021-05-24T18:25:03Z

case-lib/lib.sh

+{
+    # get the firmware type
+    manifest=/etc/sof/manifest.txt
+    FIRMWARE_TYPE=$(grep "firmwaretype" < "$manifest" | awk -F ':' '{print $2}' | sed -e 's/^[ ]*//g;s/\"//g')


No need for the <.

No need for the grep, awk can already do grep (and many other things): awk -F ':' '/firmwaretype/ {print $2 }'

Regular expressions with only special characters are time-consuming to read and deserve a comment, for instance # remove whitespace.

kv2019i · 2021-05-25T12:25:25Z

@marc-hb wrote:

@andyross , @kv2019i , @lyakh : any other, simple, automatable and reliable way to tell the difference between plain SOF versus SOF+Zephyr?

PS: I've been using a .tarball-version hack that shows up in dmesg but this won't work for CI

Not very good ones.

The compiler string has "zephyr", but that's not reliable (plain SOF could be built with zephyr toolchain, or zephyr can be built with something else).

The FW binary has a string "ZEPHYR FATAL" that you can test with "test -n "strings /lib/firmware/intel/sof/sof-tgl.ri |grep ZEPHYR FATAL""

This comes from zephyr/kernel/fatal.c , so while it's a bit adhoc, it's likely to be a pretty reliable in practise. We have code in test-case/verify-firmware-presence.sh to parse the fw file, so this is a possibility.

case-lib/lib.sh

kv2019i

I tested the patch on a system with Zephyr firmware and it did work as expected. We should make the manifest-based configuration a bit cleaner so it makes sense for standalone usage of sof-test, but otherwise this looks good.

marc-hb

I suggest we merge this with the manifest.txt first to get CI started and then try to find a better test later. Only the is_zephyr() function will have to change.

case-lib/lib.sh

marc-hb · 2021-05-25T23:25:17Z

case-lib/lib.sh

+        return 0
+    else
+        return 1
+    fi


Style nit/personal preference:

if is_zephyr || [[ ${OPT_VAL['s']} -ne 1 ]]; then return 0 fi return 1

Concise and reasonable. Updated

Actually, this can probably be just:

logger_disabled() { is_zephyr || [[ ${OPT_VAL['s']} -ne 1 ] }

case-lib/lib.sh

since zephyr doesnot support sof-logger, so add a function to disable logger trace for zephyr during the test. Signed-off-by: Zhang Keqiao <keqiao.zhang@intel.com>

aiChaoSONG · 2021-05-26T02:32:12Z

I think there's also a test -e "$manifest" || return 1 missing before the jq.

@marc-hb @keqiaozhang For CI devices, $manifest must exist, otherwise, the deployment failed, so the test is not started.
For personal device, this file may not exist. so I think we should regard '$manifest not exist' as 'zephyr not exist', and continue to check other options. this is for CI and personal device compatibility

Edit: If we just want to test if the FW is zephyr, a grep is enough, we can eliminate the dependency on jq. If we do want to parse json with jq, I think a function get_sof_type is better.

marc-hb

LGTM but probably not for some others yet.

For personal device, this file may not exist. so I think we should regard '$manifest not exist' as 'zephyr not exist',

I think the majority of people who reviewed this PR use Zephyr without a manifest.txt and at least one had a good idea that he will submit immediately after this PR is merged :-)

Edit: If we just want to test if the FW is zephyr, a grep is enough, we can eliminate the dependency on jq

jq is awesome, it's a great dependency and in this case it makes the manifest-based test very reliable. Now if the manifest.txt test goes away soon then I agree the jq dependency will be very short lived in this case...

kv2019i

Fair enough @marc-hb , let's go with this.

keyonjie · 2021-05-27T07:23:20Z

why does zephyr not support sof-logger? without FW logs, how can we debug the SOF issues?

marc-hb · 2021-05-27T07:49:21Z

We could make sof-logger compatible with Zephyr but the plan is to migrate to https://docs.zephyrproject.org/latest/reference/logging/

For now logging is provided by one of these: zephyrproject-rtos/zephyr#35186

fredoh9

didn't know sof-logger is not supported for Zephyr

marc-hb · 2021-06-01T17:12:43Z

3 approvals, 8 days in review, no formal "request changes", simple and easy to maintain change: merging.

Some very good suggestions and objections: please submit incremental changes.

marc-hb · 2021-07-01T00:02:09Z

SKIP check-sof-logger if is_zephyr #724

marc-hb · 2021-11-24T18:18:45Z

#809 just enabled sof-logger with Zephyr. Seems to work fine.

keqiaozhang requested a review from a team as a code owner May 24, 2021 09:05

keqiaozhang force-pushed the zephyr branch 2 times, most recently from 4cd89f6 to d19612a Compare May 24, 2021 09:39

marc-hb requested changes May 24, 2021

View reviewed changes

keqiaozhang force-pushed the zephyr branch 2 times, most recently from 7a6a8e9 to 436e2bd Compare May 25, 2021 09:59

lyakh reviewed May 25, 2021

View reviewed changes

case-lib/lib.sh Show resolved Hide resolved

kv2019i reviewed May 25, 2021

View reviewed changes

marc-hb requested changes May 25, 2021

View reviewed changes

logger: disable sof-logger for zephyr

206918f

since zephyr doesnot support sof-logger, so add a function to disable logger trace for zephyr during the test. Signed-off-by: Zhang Keqiao <keqiao.zhang@intel.com>

keqiaozhang force-pushed the zephyr branch from 436e2bd to 206918f Compare May 26, 2021 01:51

marc-hb approved these changes May 26, 2021

View reviewed changes

kv2019i approved these changes May 27, 2021

View reviewed changes

fredoh9 approved these changes Jun 1, 2021

View reviewed changes

marc-hb merged commit 325e572 into thesofproject:main Jun 1, 2021

marc-hb requested a review from plbossart June 1, 2021 17:16

marc-hb mentioned this pull request Jul 1, 2021

[FEATURE] sof-test should run without DMA trace - only etrace #726

Open

marc-hb added the area:logs Log and results collection, storage, etc. label Jul 3, 2021

marc-hb mentioned this pull request Nov 29, 2021

case-lib: add support for no logger mode #811

Closed

logger: disable sof-logger for zephyr #681

logger: disable sof-logger for zephyr #681

Uh oh!

Conversation

keqiaozhang commented May 24, 2021

Uh oh!

marc-hb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keqiaozhang May 25, 2021

Choose a reason for hiding this comment

Uh oh!

marc-hb May 25, 2021

Choose a reason for hiding this comment

Uh oh!

keqiaozhang May 25, 2021

Choose a reason for hiding this comment

Uh oh!

marc-hb May 24, 2021

Choose a reason for hiding this comment

Uh oh!

kv2019i commented May 25, 2021

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marc-hb May 25, 2021

Choose a reason for hiding this comment

Uh oh!

keqiaozhang May 26, 2021

Choose a reason for hiding this comment

Uh oh!

marc-hb Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aiChaoSONG commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie commented May 27, 2021

Uh oh!

marc-hb commented May 27, 2021

Uh oh!

fredoh9 left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jul 1, 2021

Uh oh!

marc-hb commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

marc-hb left a comment •

edited

Loading

marc-hb May 24, 2021 •

edited

Loading

aiChaoSONG commented May 26, 2021 •

edited

Loading

marc-hb commented Jun 1, 2021 •

edited

Loading