Skip to content
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

Checkbox 691/missing audio device (New) #622

Merged
merged 23 commits into from
Oct 12, 2023
Merged

Conversation

diohe0311
Copy link
Contributor

@diohe0311 diohe0311 commented Jul 18, 2023

Description

The problem is that the audio device cannot be detected in the RPi4. The root cause is udev_resource.py, which filters out the audio devices without a vendor ID. However, the audio devices in RPi4 do not have any vendor IDs.

After discussing with Maciej, we concluded that udev_resource is too complicated, with too many exception cases. It would be challenging to maintain and understand if we were to add more exceptions to it. Therefore, it's better to allow the audio card to have its own resource script, similar to the graphics card. Additionally, we already have alsa_pcm_info.py, which can gather information about the audio card.

Also add a unittest to testify if audio_card_resource.py can process data from file.

Changes:

  1. Replace alsa_pcm_info.py with audio_card_resource.py.
  2. Update all the test plans and jobs that required alsa_pcm_info.py.
  3. Add a unittest for audio_card_resource

Update:
At first, we thought, 'Hey, why not do a better job by (1) extracting audio resources from the udev resources? This way, udev wouldn't become more complicated. And (2) adding automated detection for playback and capture, while also removing them from the manifest, so users don't have to answer it every time.'
After serveral meetings and discussions with Pierre, Hanhsuan, Patrick and Kent, turns out manifest has great usage to QA team and OEM, its convenience and flexibility allow users to adjust the hw info efficiently. So we decide to undo the changes related to manifest and jobs requires.

Resolved issues

https://warthogs.atlassian.net/browse/CHECKBOX-691

Tests

Check flake8 and unit test
./manage.py test -f
./manage.py test -u

Check RPi4
cd checkbox/providers/
scp -r resource ubuntu@10.102.163.11:/var/tmp/checkbox-providers/
ssh ubuntu@10.102.163.11
cd /var/tmp/checkbox-providers/resource/bin
checkbox.shell
./audio_card_resource.py

Test the following jobs
checkbox-cli run com.canonical.certification::audio/[JOB_NAME]

  • audio/detect-capture-devices
  • audio/detect-playback-devices

Test with checkbox remote

  • audio/detect_sources
  • audio/detect_sinks
  • audio/alsa_record_playback_automated

failed due to known issue: #655

  • audio/alsa-loopback
  • audio/alsa-loopback-automated
  • audio/alsa-playback

https://certification.canonical.com/hardware/201911-27506/submission/343677/

@diohe0311 diohe0311 force-pushed the CHECKBOX-691/missing-audio-device branch from 461ce24 to ba58ab8 Compare July 19, 2023 03:31
@diohe0311 diohe0311 marked this pull request as ready for review July 24, 2023 11:44
@diohe0311 diohe0311 force-pushed the CHECKBOX-691/missing-audio-device branch from 34cb5f4 to 6e31f52 Compare July 31, 2023 09:09
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of the resource job is a bit unfamiliar to me but the overall functionality seems very nice!

providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in a meeting earlier with @diohe0311 , I think it's risky to remove the manifest entries (has_audio_capture and has_audio_playback). These entries are manual to allow for the tester to let Checkbox know if this device has, indeed, a sound output and/or a sound input.

If we rely solely on automated jobs, like with the audio_card resource created here, if, for any reason, the audio device(s) are not detected by Ubuntu (because of a driver issue, because of a kernel issue, because of a UEFI issue, etc.), then Checkbox will assume there are no sound input/output and will proceed with skipping the jobs. This is bad because QA and/or PM may not see that some audio jobs were skipped, and they may deem the device tests as complete whereas there should be bugs raised about the missing sound interfaces...

@kissiel, any opinion about this?

providers/base/units/audio/manifest.pxu Outdated Show resolved Hide resolved
providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments inline.

Also, I would recommend that you test your work by calling the jobs and/or the test plans you have modified. In your venv, you can run a job with, for example:

(venv) $ checkbox-cli run com.canonical.certification::audio/detect-playback-devices

(...)

==============[ Running job 2 / 2. Estimated time left: 0:00:00 ]===============
------------[ Check that at least one audio playback device exits ]-------------
ID: com.canonical.certification::audio/detect-playback-devices
Category: com.canonical.plainbox::audio
Job cannot be started because:
 - resource expression "audio_card.playback == 'supported'" evaluates to false
Outcome: job cannot be started

providers/resource/bin/audio_card_resource.py Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
@kissiel
Copy link
Contributor

kissiel commented Aug 7, 2023

As discussed in a meeting earlier with @diohe0311 , I think it's risky to remove the manifest entries (has_audio_capture and has_audio_playback). These entries are manual to allow for the tester to let Checkbox know if this device has, indeed, a sound output and/or a sound input.

If we rely solely on automated jobs, like with the audio_card resource created here, if, for any reason, the audio device(s) are not detected by Ubuntu (because of a driver issue, because of a kernel issue, because of a UEFI issue, etc.), then Checkbox will assume there are no sound input/output and will proceed with skipping the jobs. This is bad because QA and/or PM may not see that some audio jobs were skipped, and they may deem the device tests as complete whereas there should be bugs raised about the missing sound interfaces...

@kissiel, any opinion about this?

If the skip is not enough, let's mark the job with fail-on-resource flag. This way it will be very red if checkbox fails to detect any device of the type.

The problem I have right now is that we're extending general manifest questionnaire to cover some extreme corner cases that don't affect 99% of the systems (how many systems don't have any audio dev?). So for systems that are in fact audio-less, it's up to the test plan author to make sure to exclude all the audio jobs so they don't pop up as skipped/failed.

Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one unnecessary shebang (marked inline).

@scotthu27
Copy link
Contributor

Agree with @pieqq it's risky to remove the manifest entries (has_audio_capture and has_audio_playback). it usually observed the audio device not detected in develop phase.
On the other hand, there are too many different configs with desktop/ workstation in PC project, lots problem we observed by auto detection like audio_card resource here. That's why we need the manifest entries to exclude related cases in the beginning.

has_audio_capture & has_audio_playback in the manifest entries would've been for including/excluding the audio jobs.
Is it better to exclude the jobs one by one or check all jobs with fail-on-resource flag manually rather than input these 2 values in few secs?

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with QA on Mattermost on that topic, it's better to

  • leave the manual manifest
  • not use fail-on-resource flag, otherwise the jobs would be marked as failed if tester specified device has no audio input/output.

Once you've done these changes, please create a snap from your branch and try to run the following jobs (the ones that are impacted by the manual manifest) to make sure they work as expected:

  • audio/detect-capture-devices
  • audio/detect_sources
  • audio/detect_sinks
  • audio/detect-playback-devices
  • audio/alsa-loopback
  • audio/alsa-loopback-automated
  • audio/alsa_record_playback_automated
  • audio/alsa-playback

Try setting the required manifest to false, then true.

To make a snap:

  1. go to checkbox-core-snap
  2. run ./prepare.sh series22
  3. go to series22
  4. run snapcraft

If you want to create a snap for the Raspberry Pi, you need to build the arm64 version of the snap. To do that, in step 4, run snapcraft remote-build --build-for=arm64 --launchpad-accept-public-upload.

Once the snap is available, you can install it by running sudo snap install <snap-name>.snap --dangerous.

providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/audio/jobs.pxu Outdated Show resolved Hide resolved
@diohe0311
Copy link
Contributor Author

snapcraft remote-build --build-for=arm64 --launchpad-accept-public-upload

Thanks! Pierre, I tested those jobs you list on the snap built from my branch, all passed except for:

@diohe0311 diohe0311 requested review from pieqq and Hook25 August 15, 2023 06:54
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@e4fae42). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             main    #622   +/-   ##
======================================
  Coverage        ?   2.50%           
======================================
  Files           ?     144           
  Lines           ?   15838           
  Branches        ?    2773           
======================================
  Hits            ?     396           
  Misses          ?   15364           
  Partials        ?      78           

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks :)

@diohe0311 diohe0311 requested review from Hook25 and removed request for Hook25 October 12, 2023 07:13
@Hook25 Hook25 changed the title Checkbox 691/missing audio device Checkbox 691/missing audio device (New) Oct 12, 2023
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this came before the coverage requirement, I think it can be merged

@Hook25 Hook25 merged commit 970cd47 into main Oct 12, 2023
15 checks passed
@Hook25 Hook25 deleted the CHECKBOX-691/missing-audio-device branch October 12, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants