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

Soundd: move to python #30567

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Soundd: move to python #30567

merged 1 commit into from
Dec 6, 2023

Conversation

jnewb1
Copy link
Contributor

@jnewb1 jnewb1 commented Dec 1, 2023

  • match c++ impl
    • same sound scale? audible test seems equivalent between c++ and python version
  • test cpu usage
    • cpu usage is higher (1.5% -> 5%), but still reasonable considering it's now in python

system/soundd.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@jnewb1 jnewb1 force-pushed the soundd-python branch 3 times, most recently from 6685cb1 to 9c11563 Compare December 1, 2023 20:04
@adeebshihadeh
Copy link
Contributor

Looks like this doesn't need it, but if we do need it, Hardware::set_volume needs to be updated to not use pulse. If this doesn't end up needing it, let's just remove it.

std::system(("pactl set-sink-volume @DEFAULT_SINK@ " + std::string(volume_str)).c_str());

@jnewb1
Copy link
Contributor Author

jnewb1 commented Dec 1, 2023

Looks like this doesn't need it, but if we do need it, Hardware::set_volume needs to be updated to not use pulse. If this doesn't end up needing it, let's just remove it.

std::system(("pactl set-sink-volume @DEFAULT_SINK@ " + std::string(volume_str)).c_str());

yea we will just remove it and scale the sound within soundd

@jnewb1
Copy link
Contributor Author

jnewb1 commented Dec 4, 2023

  • moved sounddevice to use pulse (so we can merge before agnos)
  • compared audio before and after, seems at a similar level

@jnewb1 jnewb1 marked this pull request as ready for review December 4, 2023 20:49
@adeebshihadeh
Copy link
Contributor

Why is the CPU now 5%? Is that at idle or constantly playing a sound?

5% is fine on its own, but we should always understand why it increased and why it’s so high.

@jnewb1
Copy link
Contributor Author

jnewb1 commented Dec 4, 2023

Why is the CPU now 5%? Is that at idle or constantly playing a sound?

5% is fine on its own, but we should always understand why it increased and why it’s so high.

at idle, but its constantly streaming zeros even while idle through sounddevice.OutputStream. I can look at switching to the non-streaming api, though looping and stopping at a specific time might be a bit more tricky

@adeebshihadeh
Copy link
Contributor

Oh nice, does that make the CPU constant?


def new_alert(self, new_alert):
current_alert_played_once = self.current_alert == AudibleAlert.none or self.current_sound_frame > len(self.loaded_sounds[self.current_alert])
if self.current_alert != new_alert and (new_alert != AudibleAlert.none or current_alert_played_once):
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the same as soundd? should consider alertType, alertSize, etc.

Copy link
Contributor Author

@jnewb1 jnewb1 Dec 5, 2023

Choose a reason for hiding this comment

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

this should match the current soundd

  • let current alert finish if its playing and the alert status goes to none
  • newer (non-none) alert overrides old alert even if the old alert hasn't stopped playing

selfdrive/ui/soundd.py Outdated Show resolved Hide resolved
selfdrive/ui/soundd.py Outdated Show resolved Hide resolved
sm = messaging.SubMaster(['controlsState', 'microphone'])

with sd.OutputStream(device="pulse", channels=1, samplerate=SAMPLE_RATE, callback=self.callback):
while True:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is missing handling of a controlsState timeout

@adeebshihadeh
Copy link
Contributor

adeebshihadeh commented Dec 5, 2023

Needs to handle this:

// Handle controls timeout

We also need to add a unit test for this.

@jnewb1 jnewb1 changed the title Soundd: move to python Soundd: move to python and use alsa Dec 5, 2023
sm = messaging.SubMaster(['controlsState', 'microphone'])

if PC:
device = "pulse"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the default one on PC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 111 to 119
else:
self.update_alert(AudibleAlert.none)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to remove this branch. looks like if soundd was running at >100Hz for example, it would cut the sounds immediately after starting them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for stopping the timeout alert, I made a flag to make that more clear and also more robust if we are running >100hz

assert wavefile.getframerate() == SAMPLE_RATE

length = wavefile.getnframes()
self.loaded_sounds[sound] = np.frombuffer(wavefile.readframes(length), dtype=np.int16).astype(np.float32) / 32767
Copy link
Contributor

Choose a reason for hiding this comment

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

what's 32767?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2^16/2 for converting the int16 to float. I put that in

AudibleAlert.warningImmediate: ("warning_immediate.wav", None, MAX_VOLUME),
}

def check_controls_state(sm):
Copy link
Contributor

Choose a reason for hiding this comment

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

make this name more specific

@jnewb1 jnewb1 changed the title Soundd: move to python and use alsa Soundd: move to python Dec 5, 2023
@jnewb1 jnewb1 force-pushed the soundd-python branch 3 times, most recently from 666f0bb to 2311290 Compare December 6, 2023 00:51
@jnewb1 jnewb1 merged commit abe39e5 into master Dec 6, 2023
22 of 23 checks passed
@jnewb1 jnewb1 deleted the soundd-python branch December 6, 2023 02:10
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.

2 participants