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

fix lipsync.h compile error #77

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mongonta0716
Copy link
Sponsor Collaborator

This is an omission that was corrected when the library was changed to M5Unified.

#76

  • [ x ] Bug fix

@mongonta0716
Copy link
Sponsor Collaborator Author

Updated to M5Unified-compatible audio and talk samples.

Copy link

@yoshipon yoshipon left a comment

Choose a reason for hiding this comment

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

(Sorry for the inconvinience. I deleted my duplicated comment.)

Copy link

@yoshipon yoshipon left a comment

Choose a reason for hiding this comment

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

Thank you for the library migration!!!
I think simplicity is important for beginners.
So, if we don't need the custom configuration for Speaker, I think it is better to leave the default configuration.

What do you think?
Thank you in advance!

Comment on lines +73 to +74
auto cfg = M5.config();
M5.begin(cfg);

Choose a reason for hiding this comment

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

Suggested change
auto cfg = M5.config();
M5.begin(cfg);
M5.begin();

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

I think this one should remain.

Copy link

@yoshipon yoshipon May 28, 2022

Choose a reason for hiding this comment

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

Until the M5Unified's documentation is more solid, I'll try to supplement it with a few comments.

Like this? (this is just a suggestion based on your kind information on twitter👍)

Suggested change
auto cfg = M5.config();
M5.begin(cfg);
auto cfg = M5.config(); // modify cfg for configurating M5 (details: https://github.com/m5stack/M5Unified/blob/master/examples/Basic/HowToUse/HowToUse.ino#L47-L60)
M5.begin(cfg);

# すみません,お仕事増やしてしまい恐縮なので,適当にコメント考えてみました! ご参考まで

Comment on lines +79 to +84
{ /// custom setting
auto spk_cfg = M5.Speaker.config();
/// Increasing the sample_rate will improve the sound quality instead of increasing the CPU load.
spk_cfg.sample_rate = 96000; // default:64000 (64kHz) e.g. 48000 , 50000 , 80000 , 96000 , 100000 , 128000 , 144000 , 192000 , 200000
M5.Speaker.config(spk_cfg);
}

Choose a reason for hiding this comment

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

Suggested change
{ /// custom setting
auto spk_cfg = M5.Speaker.config();
/// Increasing the sample_rate will improve the sound quality instead of increasing the CPU load.
spk_cfg.sample_rate = 96000; // default:64000 (64kHz) e.g. 48000 , 50000 , 80000 , 96000 , 100000 , 128000 , 144000 , 192000 , 200000
M5.Speaker.config(spk_cfg);
}

@mongonta0716
Copy link
Sponsor Collaborator Author

Speaker(spk_cfg) may not need it, but as for M5.begin(), beginners are not familiar with M5Unified's Config method, so it is better to write it.

@yoshipon
Copy link

Thanks for your rapid reply! I see!!!
Kind tutorials are most important :)

@mongonta0716
Copy link
Sponsor Collaborator Author

Yes, it is. It is indeed hard to understand.

Until the M5Unified's documentation is more solid, I'll try to supplement it with a few comments.

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.

None yet

2 participants