Support loading patches

[baldram]

Hi All,
Out of a clarification from #65 (comment) I open a discussion regarding adding a support for loading patches (from http://www.vlsi.fi/en/support/software/vs10xxpatches.html).

An example from @CelliesProjects repository is here: vs1053b-patches.plg.h.
This is v2.9 from http://www.vlsi.fi/fileadmin/software/VS10XX/vs1053b-patches290.zip
From that vs1053-patches.plg and renamed to vs1053-patches.plg.h.

Now, we would like to integrate a support into ESP_VS1053_Library.
As discussed here: #65 (comment), @CelliesProjects is willing to code a PR for that.

Now, a question:

1. Shall we ship patches code inside ESP_VS1053_Library, then all vs1053 related (VS1053b Patches w/ FLAC Decoder, VS1053b Realtime MIDI Start code, VS1053b IMA ADPCM Encoder Fix, ...)?
   Or patches should be rather a part of project using ESP_VS1053_Library?
2. Would be enough to deliver only a universal loadUserCode method? Or both a method and patches?
3. If about loadUserCode implementation, that would be something like here (@CelliesProjects please correct me if I'm wrong):
   https://github.com/CelliesProjects/ESP32_VS1053_Stream/blob/master/src/ESP32_VS1053_Stream.cpp#L440
   ...but instead of loadUserCode(void) that would have the argument loadUserCode(const unsigned short plugin[]), so that we could use it for any patch code according to needs.
   Of course we would have to check whether all official patches are loaded using similar code snipped. That this can be universal.
4. Any other open points?

The "loading patches" topic is also connected with the following issue #52 as patch has improvements on different things which might help to suppress the glitches and other streaming troubles.
Looking to patch's change log, we notice eg. "ID3 tag parser skips the tag data, so that binary data inside the tag is not erroneously interpreted as audio", what was one of issues from past discussions if I correctly remember.

CC: @wmarkow @fabitom @philippedc @ewbarnes53 @DindonCurieux @Dr-Dawg @Edzelf @vincegellar

What do you think?

[baldram]

Summarizing above...

For implementing a better Internet Radio we might consider a data patch (aka plugin) feature to apply these patches. Those are VS1053b Patches v2.9 and they help to get rid of some streaming problems, as well as handle ID3 tag things better (so that binary data inside the tag is not erroneously interpreted as audio), and so on.

A question is whether we would like to:

• implement only a new loadUserCode(const unsigned short plugin[]) method allowing to apply any patch (I think we can do it ASAP anyway),
• implement above mentioned method and also store some (or all?) patches in the latest version inside a library; then a user would be able to pick which to load in his/her application.

Would you please leave your thoughts on it?

[CelliesProjects]

I think the default (no user interaction) should be load the generic firmware patch. (The patch used in ESP32_VS1053_Stream) This should solve a lot of issues for most use cases.

I was thinking maybe put a single entry in VS1053.h something like:

#include "patch_select.h"

And then in patch_select.h the plugins are listed (so you can use comments and expand the list without cluttering up VS1053.h):

/* pluginA is the latest greatest.... */
#include "pluginA.plg" 

/* pluginB comes with a caveat.... */
//#include "pluginB.plg" 

//#include "pluginC.plg" 

//#include "pluginD.plg" 

With pluginA.plg being the default latest firmware patch. No interaction needed except if you need a particular patch instead of the latest generic firmware.

Not very sophisticated but I like KISS.

[baldram]

With pluginA.plg being the default latest firmware patch. No interaction needed except if you need a particular patch instead > of the latest generic firmware.

Not very sophisticated but I like KISS.

Thank you @CelliesProjects for sharing your proposal.
I like the idea of a default plugin that solves most issues and doesn't require any additional user interaction.

Not sure I see correct the idea of commented code. I would not use commented code, as inside a library, it doesn't bring any value.
But in the first step, we might focus on a "default" patch, preserving the possibility of loading any custom one manually.

So we have the patch_select.h or we might even name it simpler (and intuitive) plugins.h.
Now, we might put there a default one, calling it properly vs1053bPatches (in same way as the vendor names it, with a code comment describing a version for clarity).

I mean that instead of

#ifndef SKIP_PLUGIN_VARNAME
const unsigned short plugin[] = { /* Compressed plugin */
#endif

that would be

// VS1053b Patches v2.9 (http://www.vlsi.fi/fileadmin/software/VS10XX/vs1053b-patches290.zip)
#ifndef SKIP_PLUGIN_VARNAME
const unsigned short vs1053bPatches[] = { /* Compressed plugin */
#endif

The patch would be loaded, using a similar way as you did here:
https://github.com/CelliesProjects/ESP32_VS1053_Stream/blob/master/src/ESP32_VS1053_Stream.cpp#L433
This also would be executed in a similar way as here:
https://github.com/CelliesProjects/ESP32_VS1053_Stream/blob/master/src/ESP32_VS1053_Stream.cpp#L60

The only difference would be that instead of calling it like this loadUserCode() it would be loadUserCode(vs1053bPatches).
Now, we could introduce the additional method loadDefaultVs1053Patches(). This would be a KISS version, that would simply call loadUserCode(vs1053bPatches). A kind of alias. Still, the user will be able to use loadUserCode with any patch of choice.

But a default use case will be as simple as that:

vs1053->begin();
vs1053->loadDefaultVs1053Patches();
vs1053->switchToMp3Mode();

That would be also documented in README.md.

If someone has a custom use case, might do like this:

// assuming that in own project there is some `vs1053b-rtmidistart.h`
vs1053->begin();
vs1053->loadUserCode(vs1053bRtMidiStart);

What do you think about this approach?

PS: That's good we made write_register public, as some plugins might require activation like discussed here.
Even it's loaded in same way as in your case.
Someone tried to implement void VS1053::ActivatePlugin(void)
As documentation says "Activate the application by writing 0x34 to register SCI_AIADDR (10)".

[CelliesProjects]

I now get your more of the gist of your idea. Sound good.

So patches would be added as a file or files with the plugin var renamed to the plugin function. I got that right?

Yes keeping write_register public is also preferred by me.

The factory patches all write 0x50 to AIADDR as last write and so auto reset the decoder. See vs1053b-patches.pdf page 4.
(If I get that right, pdf is a bit vague there, but the last wite in the .plg is a empty(?) write to ox50)

A software reset will erase the patch and the reset routine should maybe be changed. (did not look at that code though) Also see vs1053b-patches.pdf page 4.

[baldram]

So patches would be added as a file

Actually files instead of file is a good idea. So a file per plugin. They might be in a separate folder.
For now, only a single one would be considered, a default plugin in patches/vs1053bPatches.h (within a stuff from vs1053b-patches290.zip) .

...with the plugin var renamed to the plugin function. I got that right?

Exactly. So vs1053bPatches var name for a default one, which is the only one considered at the moment.
Later, in the future, more plugins/patches can be added in same way.

The factory patches all write 0x50 to AIADDR as last write and so auto reset the decoder.

It seems to be like this. In case of factory patches they say "These patches use the application address to start automatically...", sounds like this.
What is good, there will be a public write_register for other patches, where someone might execute it according to needs and/or requirements.

Also, what is good, the whole concept doesn't break backward compatibility, as calling vs1053->loadDefaultVs1053Patches() would be still only an option that someone might optionally call in his/her code. However, a default example from README.md will clarify that using it is highly advised.

I think this is quite a universal approach, and also it follows your idea of a KISS solution that uses the latest firmware patch by default, which solves many issues for most use cases.

[baldram]

A functionality delivered! 🎉
#69

Below is a summary of what was delivered.

Added an extension for dealing with vs1053 Firmware patches (binary patch files that can easily be loaded to the memory of VS10XX by a microcontroller to fix known firmware bugs):

• enable a possibility for using patches/plugins,
• load a default patch,
• include the latest available patches from http://www.vlsi.fi/en/support/software/vs10xxpatches.html
• update example code and documentation,
• adjust a functionality for low-level writeRegister.

Thank you @CelliesProjects for your effort!

[Dr-Dawg]

I just tested the new version, which raised two questions:

1. I had to rename vs1053b-patches.plg to vs1053b-patches.h in order for the code to compile. Is this a matter of using Arduino IDE rather than PlatformIO?
2. The VS1003/1053 labeled blue LC Technology modules like the ones baldram shows in the hardware pictures contain a VS1003 chip to the best of my knowledge. The player.loadDefaultVs1053Patches() command by default loads the 2.9 VLSI patch for the VS1053. When testing with the VS1003 module, I expected the board not to play any sound, but it did. So am I right that when I apply the "wrong" patch no harm is done "accidently"?

[Dr-Dawg]

Sorry, harm was the wrong word - I didn't expect any permanent damage, otherwise I wouldn't have tested the patch on the VS1003 :-)
It's more a point to the discussion whether you want one code for all modules (in this case commenting the loadDefaultVs1053Patches() command, or creating a loadDefaultVs1003Patches() command might be a good alternative to applying the 1053 patch to all modules )
or to generate some sort of switch depending on the VLSI module being used.

Since you are heading towards RTMidi, I'd like to draw your attention to the project of @diyelectromusic

https://github.com/diyelectromusic/sdemp/tree/master/src/SDEMP/ArduinoMIDIVS10xxSynth

He put a lot of effort in making Midi work for both mudules, using a switch like

#define VS1003_MODULE 1

#ifdef VS1003_MODULE
extern "C" {
#include "rtmidi1003b.h"
#include "vs1003inst.h"
}
#endif
#ifdef VS1053_MP3_SHIELD
extern "C" {
#include "rtmidi1053b.h"
}
#endif

He also had a close look at existing solutions like the one mentioned by @knopserl .
I haven't tested that one yet, but since I definitly want to implement Midi functionality (that was also the reason for me to add a gyroscope to the webradio) for both modules, this will be my next step.

[baldram]

It's more a point to the discussion whether you want one code for all modules (in this case commenting the loadDefaultVs1053Patches() command, or creating a loadDefaultVs1003Patches() command might be a good alternative to applying the 1053 patch to all modules )
or to generate some sort of switch depending on the VLSI module being used.

For sure, not all modules by default. Only necessary ones. It would be nice to cover "Fixes in Details" chapter from vs1053b-patches.pdf.
The goal of loadDefaultVs1003Patches is to load only necessary patches from VLSI, related to the default firmware of the vs1053 device (fixing factory bugs, etc). Those are delivered via "VS1053b Patches w/ FLAC Decoder" one from here: http://www.vlsi.fi/en/support/software/vs10xxpatches.html.

This should fix some problems, eg. with glitches, etc., that people had when implementing web radio. The vs1053b-patches290.zip is the one we expect to load by default (still optionally, someone might load it or not - depending on the use case).
Not sure the vs1053b-patches.plg is enough, as the zip contains more, and a question is which ones from vs1053b-patches290.zip require loading by default? Do you have some observations on it?

I'm not sure I got "loadDefaultVs1053Patches() command, or creating a loadDefaultVs1003Patches() command might be a good alternative". That this method would load everything (*.plg) from vs1053b-patches290.zip or what do you mean? Or that we need 1003 alternative that would load a content of "VS1003 DAC Mono Patch" and/or "WAVFix Patch" (or some other one, which one)? That's a good idea I think. That's why no default actions and it's loaded on user's choice explicitly.

whether you want one code for all modules

We don't plan to load by default patches from other ZIP files, like MIDI, etc. Those are to be loaded optionally.
There is loadUserCode(anyPatch) method to load optional extensions.
(We will need to update documentation for more details, that's right).

As mentioned, I think those from vs1053b-patches290.zip should be considered whether a single one or more should be loaded by default?

Since you are heading towards RTMidi, I'd like to draw your attention to the project of @diyelectromusic

Thanks! I think we will achieve it by delivering the necessary minimal changes in a library.
Probably by adding some small (potentially missing) things and a basic example.
A MIDI feature will be only the option.

PS: Ping @CelliesProjects 😏 Still an open question #66 (comment) whether you're fine we will rename all plg to h?

[CelliesProjects]

@Dr-Dawg Thanks for your observation and reporting this issue.

@baldram Yeah of course.
I am a bit busy IRL so I did not even look at the code.

[baldram]

@CelliesProjects Thanks for reply. Good luck IRL.

whether you want one code for all modules

@Dr-Dawg I think by modules you meant "devices" and I meant "patches/extensions". 😃
If you think only about adding loadDefaultVs1003Patches that's fine. Please have a look at this comment #66 (reply in thread) and references to VLSI site there, and tell which exactly patch file/files you mean as "default" for 1003?

We might also add more patch files to the library and point user in the documentation to load (optionally) chosen ones via loadUserCode(anyPatch) method which is available today.

What do you think?

[Dr-Dawg]

I think by modules you meant "devices" and I meant "patches/extensions". 😃

You are right. And you were also right with

Or that we need 1003 alternative that would load a content of "VS1003 DAC Mono Patch" and/or "WAVFix Patch" (or some other one, which one)?

These are also the only 1003 patches I saw, and they're not really needed, so my suggestion for a loadDefaultVs1003Patches command is obsolete. I think pointing the user to the VLSI site and the possibility of loading plugins/patches is a good idea.

[Dr-Dawg]

Back to what I meant with comments: Most of the 1003 and 1053 devices that are sold completely look the same. They are even both labeled with "VS1003/1053 MP3 CODEC". The only way to distinguish them the is the colour (green = VS1053, blue = VS1003, I'm not sure about the red ones) or having good eyes and a close look at the chip. That means a lot of users use a VS1003 chip without even being aware of it.

So, even if it works with all devices, I still feel sort of uncomfortable with the line

player.loadDefaultVs1053Patches();

in the WebRadioDemo. I'd rather leave a comment, something like

player.loadDefaultVs1053Patches(); // only use this if you are sure you are using a vs1053 device, e.g. green devices, Adafruit or Sparkfun Shield. Blue devices contain a vs1003 chip

or

// player.loadDefaultVs1053Patches(); // uncomment this if you are sure you are using a vs1053 device, e.g. green devices, Adafruit or Sparkfun Shield. Blue devices contain a vs1003 chip

[baldram]

Thanks @Dr-Dawg for telling about this!

That means a lot of users use a VS1003 chip without even being aware of it.

OMG! That's me! I was not aware of this problem and I have a blue one. Fortunately it is VS1050B. So if about the following....

The only way to distinguish them the is the colour (green = VS1053, blue = VS1003, I'm not sure about the red ones)

Does it mean that, in your case, the blue one is VS1003? Then this is not that simple that we can distinguish them by colour.

Please see mine:

And yes, it is indeed described as VS1005/10003 as you showed.

in the WebRadioDemo. I'd rather leave a comment, something like ... or ... uncomment

Yes, I would follow the "uncomment this if..." approach.
As it's not that simple that we can distinguish them by color, I would add a sentence like this.
"uncomment this if you are sure you are using a vs1053 device, see README.md for more details on how to distinguish them".

Then in read me we will need probably to add a section pointing to this problem, adding some photos of the chip plus some clarification that branded ones like Adafruit or Sparkfun are rather trustworthy. If about non-branded ones from Aliexpress, we need to be careful and check the chip label.

Thanks for pointing to this problem! If you're willing to make a MR and update documentation (and those comments in examples), please feel free.
That would be a section in Readme somewhere below "Example wiring" and before this: https://github.com/baldram/ESP_VS1053_Library#library-developement
Just asking, if you like to add this MR, as it's your finding about this important problem.
I feel also uncomfortable even if it works with all devices as you said.

[Edzelf]

Why note read the SCI_STATUS register. Bits 0..2 will tell you the version:
0 = VS1001, 1 = VS1011, 2 = VS1002, 3 = VS1003, 4 = VS1053, 6 = VS1103

[baldram]

Why note read the SCI_STATUS

Very good point. Thanks @Edzelf!
We might simply add a condition to loadDefaultVs1053Patches method. 👍

Anyway, in examples, probably it's worth to mention in a comment that the loadUserCode method allows to load any custom patch, eg. for VS1003 module.

[CelliesProjects]

@Edzelf @baldram The vs1053 pdf (page41) says:

SS_VER bits 7:4

SS_VER is 0 for VS1001, 1 for VS1011, 2 for VS1002, 3 for VS1003, 4 for VS1053 and VS8053,
5 for VS1033, 7 for VS1103, and 6 for VS1063.

So that should be bits 7 through 4.

Note that id 4 is used for both 1053 and 8053 chips.

[Dr-Dawg]

Gosh, I'm pretty new to this GitHub thing / merge/ pull/ whatsoever ..

I (hopefully) just proposed two pull requests adding a method to get the Chip number as well as a method for switching the VS10xx Stream mode on and off..

[Dr-Dawg]

With this, it should be possible to do something like

    if(player.getChipVersion()==4) // Only perform an update if we really are using a VS1053
        player.loadDefaultVs1053Patches();   

[baldram]

Gosh, I'm pretty new to this GitHub thing / merge/ pull/ whatsoever ..

Ok, I see. Will have a look into this later. I think I will help squash those two into a single MR.
Those are a single thing 😄

[Dr-Dawg]

Well, I think I got it now..

[Dr-Dawg]

As mentioned in #32, I think there's a bug in the loadUserCode function (and yes, I know, it's from VLSI and they did well ;-) ). The array is passed as call by reference. I.e. sizeof(plugin) delivers the size of the adress,
hence sizeof(plugin)/sizeof(plugin[0] returns 4/2 = 2, so only roundabout two values (and some copy runs) are written.
I just checked the _loadUserCode(void) code from @CelliesProjects before switching to the VS1053 library and it worked due to the fact that no parameters were passed.
However, after patching VS1053 by the driver, I think also no updates take place in the code of @CelliesProjects.

@baldram @CelliesProjects could you check whether I'm on the right path? I'm preparing a pull request to fix this.

[fixus971]

Yes! My updated compiler put a big warning here!
You have to return to a function without par or add size par:

void function( int *arr, size_t sz )
  { ...  ...  ... }
:
{
    int x[200];
    function( x, sizeof(x) / sizeof(*x) );
}