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

Create the Shopping Cart Lock app #2326

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

RocketGod-git
Copy link
Contributor

Will demonstrate tomorrow. Don't merge until I do 😁

Will demonstrate tomorrow. Don't merge until I do 😁
Copy link
Member

@htotoo htotoo left a comment

Choose a reason for hiding this comment

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

lgtm

htotoo
htotoo previously requested changes Oct 30, 2024
Copy link
Member

@htotoo htotoo left a comment

Choose a reason for hiding this comment

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

but only after code format :) (forgot that part)

@gullradriel
Copy link
Member

Looks fine, @RocketGod-git tell us if you need help for the formatting.
Follow the guide here: https://github.com/portapack-mayhem/mayhem-firmware/wiki/Code-formatting
Ask us if you're blocked somewhere

@RocketGod-git
Copy link
Contributor Author

No worries on the formatting. I'm just working on the sound quality right now. If not correct it won't work so refining the code to pull exact properties from the audio metadata in the wav file. Also, for a future project - but I won't merge until I think it's perfect. Hopefully this weekend.

If nobody has any ideas to further improve high frequencies of the audio, the hardware may not be capable. I still need to check with line-out to better speaker to make sure it's not just the speaker, but it shouldn't be.
…ill playback seems to be missing higher fq sounds
@ArjanOnwezen
Copy link
Contributor

Really cool @RocketGod-git
Is this similar to https://www.begaydocrime.com/ ?

@RocketGod-git
Copy link
Contributor Author

Yes. I stole their wav files 🙃

@ArjanOnwezen
Copy link
Contributor

Yes. I stole their wav files 🙃

They also have two more WAV files, but we don't have that system in The Netherlands, hence can't test it :)

@gullradriel
Copy link
Member

I would also rename the wav files to something like "cart_lock" and "cart_unlock" to be less generic than "lock" and "unlock"

@gullradriel
Copy link
Member

Okay, I did review it fully.

As I don't have write access to your branch on your repo, I can only ask you to make the changes.

Please add these definitions in shoppingcart_lock.hpp

const std::string shoppingcart_lock_file{"shopping_cart_lock.wav"};
const std::string shoppingcart_unlock_file{"shopping_cart_unlock.wav"};

Please replace all occurences of lock.wav by shoppingcart_lock_file
Please replace all occurences of unlock.wav by shoppingcart_unlock_file

Please rename sdcard/WAV/lock.wav to sdcard/WAV/shopping_cart_lock.wav
Please rename sdcard/WAV/unlock.wav to sdcard/WAV/shopping_cart_unlock.wav

Please reindent all project using format-code.sh at the root of the repository

Commit all this and we will be able to merge :-)

@gullradriel
Copy link
Member

Ho well. I did it for you

Copy link
Member

@gullradriel gullradriel left a comment

Choose a reason for hiding this comment

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

Looks good to me

@gullradriel gullradriel dismissed htotoo’s stale review November 8, 2024 15:06

indent have been done. can't merge without dismissing the requested change

@gullradriel gullradriel merged commit 09dff44 into portapack-mayhem:next Nov 8, 2024
3 checks passed
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
htotoo pushed a commit to htotoo/portapack-mayhem that referenced this pull request Nov 11, 2024
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.

4 participants