Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improvement: the setVolume() should write volume every time it is called #26

Closed
wmarkow opened this issue Oct 14, 2018 · 3 comments
Closed

Comments

@wmarkow
Copy link

wmarkow commented Oct 14, 2018

Currently setVolume(uint8_t vol) uses a cache mechanism and will not write the same value for the second time. In my project I want to deal with some hardware failures (like for example hot removing the VS1053 when NodeMCU is still working). Those cases can be identified by checking the output of isChipConnected() introduced in #24.

In such a cases I would like to reinit the VS1053 chip by calling begin() again but during begin() calls bool testComm(const char *header) which modifies the volume and it is hard to set volume again. In other words, the code below will make that the volume will not be set (a workaround helps in this case):

begin();        // this will reinit the module and "break" the volume
setVolume(20);  // this will set the volume to 20. The library will cache this value for future use.
begin();        // this will reinit the module and "break" the volume
setVolume(20);  // this will not set the volume at all, because the library thinks that the volume was previously set to 20
setVolume(19);  // as a workaround I need to set a different value of volume...
setVolume(20);  // ... and then back again to 20 and it works

Also if you power off the VS1053 and then power it up again (when NodeMCU is still working) then it will be not possible to set the same volume again.

My suggestion for this library is: when setVolume is called then set the volume always. In other words the cache mechansim could (or should) be removed.

@baldram
Copy link
Owner

baldram commented Oct 16, 2018

Hi @wmarkow
Thank you for your feedback. It has been introduced by the author of Esp-radio (this commit: Edzelf@38a9afa#diff-fcb767b1d70e7746c6d39dc221e647b1) for unknown reason. I don't see any explanation.
I think this cache may be removed and hopefully without any side effect.

Would you like to send a pull request to solve this issue?

@wmarkow
Copy link
Author

wmarkow commented Oct 19, 2018

Would you like to send a pull request to solve this issue?

PR prepared.

@baldram
Copy link
Owner

baldram commented Oct 20, 2018

Thank you for the pull request. Merged.
I will perform a smoke test before releasing the latest changes.

Btw. please do not forget to star the project if you like it :-)

@baldram baldram closed this as completed Oct 20, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants