offa / plug

Software for Fender Mustang Amps.
GNU General Public License v3.0
54 stars 20 forks source link

App crash on connection/library view #26

Closed krskajo1 closed 2 months ago

krskajo1 commented 2 months ago

Hi, whenever the Plug tries to connect to my Fender Mustang I V2 it crashes with nothing on the output but the following error

/usr/include/c++/14.1.1/bits/stl_vector.h:1149: std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::operator[](size_type) const [with _Tp = std::__cxx11::basic_string<char>; _Alloc = std::allocator<std::__cxx11::basic_string<char> >; const_reference = const std::__cxx11::basic_string<char>&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)

Same thing happens when I try to open the "Library View".

The culprit is in the functions load_names in ui/library.cpp:54, ui/loadfromamp.cpp:64, ui/quickpresets.cpp:53 and ui/saveonamp.cpp:65. All the functions try to load the input vector of effect names into a visual component and try to access elements beyond its actual size. The loop's upper limit is a hard-coded 100 instead of the actual size of the vector. The 100 is the capacity of the vector, however, and is set during its creation. I'm not sure if the constant is there on purpose, but my amp only has 24 effects so I can't use the app without fixing this. I did the following changes to the code

diff -ura plug.orig/src/ui/library.cpp plug.new/src/ui/library.cpp
--- plug.orig/src/ui/library.cpp    2023-12-05 20:25:19.000000000 +0100
+++ plug.new/src/ui/library.cpp 2024-09-12 19:30:45.053052939 +0200
@@ -51,7 +51,7 @@
         ui->spinBox->setValue(font.pointSize());
         ui->fontComboBox->setCurrentFont(font);

-        for (std::size_t i = 0; i < 100; ++i)
+        for (std::size_t i = 0; i < names.size() && i < 100; ++i)
         {
             if (names[i][0] == 0x00)
             {
diff -ura plug.orig/src/ui/loadfromamp.cpp plug.new/src/ui/loadfromamp.cpp
--- plug.orig/src/ui/loadfromamp.cpp    2023-12-05 20:25:19.000000000 +0100
+++ plug.new/src/ui/loadfromamp.cpp 2024-09-12 19:30:45.053052939 +0200
@@ -61,7 +61,7 @@

     void LoadFromAmp::load_names(const std::vector<std::string>& names)
     {
-        for (std::size_t i = 0; i < 100; ++i)
+        for (std::size_t i = 0; i < names.size() && i < 100; ++i)
         {
             if (names[i][0] == 0x00)
             {
diff -ura plug.orig/src/ui/quickpresets.cpp plug.new/src/ui/quickpresets.cpp
--- plug.orig/src/ui/quickpresets.cpp   2023-12-05 20:25:19.000000000 +0100
+++ plug.new/src/ui/quickpresets.cpp    2024-09-12 19:30:45.053052939 +0200
@@ -50,7 +50,7 @@
         QSettings settings;
         std::size_t i = 0;

-        for (i = 0; i < 100; i++)
+        for (i = 0; i < names.size() && i < 100; ++i)
         {
             if (names[i][0] == 0x00)
             {
diff -ura plug.orig/src/ui/saveonamp.cpp plug.new/src/ui/saveonamp.cpp
--- plug.orig/src/ui/saveonamp.cpp  2023-12-05 20:25:19.000000000 +0100
+++ plug.new/src/ui/saveonamp.cpp   2024-09-12 19:30:45.053052939 +0200
@@ -62,7 +62,7 @@

     void SaveOnAmp::load_names(const std::vector<std::string>& names)
     {
-        for (std::size_t i = 0; i < 100; ++i)
+        for (std::size_t i = 0; i < names.size() && i < 100; ++i)
         {
             if (names[i][0] == 0x00)
             {

Thank you for resolving this issue.

PS: Thanks for this project! I'm glad I can use the amp with linux.

offa commented 2 months ago

Thanks for your report. Instead of testing for both limits names.size() && i < 100 it might be enough to use the former only. It shouldn't contain more entries.

krskajo1 commented 2 months ago

Hi. Yes, I would agree using only the first limit. I left the i<100 in there in case it was important not to process more than 100 entries.

offa commented 2 months ago

Good change to replace some more raw loops. Could you give the 293-mustang_i_crash branch a try? Working on a 100 preset device, but better test it with 24 too.

krskajo1 commented 2 months ago

Thanks! The new version seems to work well with the amp. It does not crash and it controls the amp. So this solved this issue.

To be complete however, I now noticed of these warnings when opening the "Library View" (the warnings were there also before the fix)

qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
  QListWidget::currentRowChanged(int) --> plug::Library::load_slot(std::size_t)
qt.core.qobject.connect: QObject::connect: Incompatible sender/receiver arguments
  QListWidget::currentRowChanged(int) --> plug::Library::load_file(std::size_t)

and these errors when changing to specific presets.

ERROR:  Invalid amp id: 246
ERROR:  Invalid amp id: 246
ERROR:  Invalid amp id: 255
ERROR:  Invalid amp id: 246

Mustang I V2 has 24 presets split into three banks by 8 presets each. I got the error when changing to some effects from either second or third bank.

Either of these problems seem not to be related to the crashing issues and could be added to a new issue.

Thanks for resolving the problem!

offa commented 2 months ago

I have merged the fix into master.

The other two issues are different problems.

The Incompatible sender/receiver arguments warning might be caused by incompatible types (due to Qt 5 -> Qt 6 upgrade?).

Can you figure out what amp models the ID's are? None is listed here, thus the error. Could these models be Mustang I (v2) specific?

krskajo1 commented 2 months ago

Great. Ok thanks for the explanation. I will check it tomorrow, I'm out of time for today. So far I found out that the Mustang I V2 has 16 amps, while the app lists only 12. I will expand on that tomorrow.

krskajo1 commented 2 months ago

Hi again. I tried to identify the missing amps (apparently there are 17 amps in total on the Mustang I V2). If did it correctly the missing amps and ids should be:

The names can also be found in the Fender Mustang user manual (page 7). The information there holds true for Mustang I despite it being in the section for Mustang III/IV/V.

offa commented 2 months ago

I have just fixed the Incompatible sender/receiver arguments warning on master.

One thing you can give a try is to replace an existing id in the code linked above and see whether it'll work.

To send those amps back to the device we need additional 'unknown' values, which purpose or meaning I haven't found out yet :sweat_smile: (see here. But that shouldn't be too difficult to figure out.

krskajo1 commented 2 months ago

Hello, below are the 'unknown' values for each new amp. The USB packets setting the amp are listed as well. BTW: Do you have at least some clue what any of the 'unknown' bytes means? The last one in the calls sure looks suspiciously like an ID of the following AMP. But I have no idea why that would be helpful... maybe for some lists?

Studio Preamp (0xf1), 'unknown' 0x0d, 0x0d, 0x0d, 0x0d, 0xf6

0000   1b 00 10 10 1e 2e 0b c2 ff ff 00 00 00 00 09 00
0010   00 01 00 05 00 01 01 40 00 00 00 1c 03 05 00 00
0020   00 01 01 00 00 00 00 00 00 00 00 f1 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 ff 81 81 81 81
0040   81 81 81 81 81 81 81 0d 0d 0d 00 00 00 0d 01 00
0050   01 f6 00 00 00 00 00 00 00 00 00               

Fender '57 Twin (0xf6), 'unknown' 0x0e, 0x0e, 0x0e, 0x0e, 0xf9

0000   1b 00 10 90 fd 2e 0b c2 ff ff 00 00 00 00 09 00
0010   00 01 00 05 00 01 01 40 00 00 00 1c 03 05 00 00
0020   00 01 01 00 00 00 00 00 00 00 00 f6 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 ac 4e 81 81 ac
0040   7b 9d bd 81 81 81 81 0e 0e 0e 00 00 09 0e 01 00
0050   01 f9 00 00 00 00 00 00 00 00 00               

60s Thrift (0xf9), 'unknown' 0x0f, 0x0f, 0x0f, 0x0f, 0xfc

0000   1b 00 70 99 e5 2d 0b c2 ff ff 00 00 00 00 09 00
0010   00 01 00 05 00 01 01 40 00 00 00 1c 03 05 00 00
0020   00 01 01 00 00 00 00 00 00 00 00 f9 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 ac 81 81 81 ac
0040   81 9d 81 81 81 81 81 0f 0f 0f 00 00 01 0f 01 00
0050   01 fc 00 00 00 00 00 00 00 00 00               

British Colour (0xfc), 'unknown' 0x10, 0x10, 0x10, 0x10, 0xff

0000   1b 00 70 a3 46 2e 0b c2 ff ff 00 00 00 00 09 00
0010   00 01 00 05 00 01 01 40 00 00 00 1c 03 05 00 00
0020   00 01 01 00 00 00 00 00 00 00 00 fc 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 ac 8c 81 81 9d
0040   64 81 81 81 81 81 81 10 10 10 01 00 08 10 00 00
0050   01 ff 00 00 00 00 00 00 00 00 00               

British Watts (0xff), 'unknown' 0x11, 0x11, 0x11, 0x11, 0x00

0000   1b 00 a0 d9 7a 2f 0b c2 ff ff 00 00 00 00 09 00
0010   00 01 00 05 00 01 01 40 00 00 00 1c 03 05 00 00
0020   00 01 01 00 00 00 00 00 00 00 00 ff 00 00 00 00
0030   00 00 00 00 00 00 00 00 00 00 00 ba 9d 81 ff ba
0040   98 a6 87 81 81 81 81 11 11 11 00 00 0a 11 01 00
0050   01 00 00 00 00 00 00 00 00 00 00
offa commented 2 months ago

Thanks, I'll have a look at it.

BTW: Do you have at least some clue what any of the 'unknown' bytes means?

No, but to be honest, I haven't invested too much time in it. At least a pattern is that there are 4 times same values, followed by a different one.

offa commented 2 months ago

I have added the Studio Preamp for testing on the 297-mustang_I_v2 branch. Could you give it a try?

krskajo1 commented 2 months ago

Hi, thanks, the change works and I now longer get the "Invalid amp" error when the amp is set to "Studio Preamp". But the combobox in Amplifier window is empty and I can not switch to it with Plug. At least the amp is now correctly recognized.

offa commented 2 months ago

Forgot the UI entry; branch updated.

I wonder why cabinets are all off in the listed packets though!? First 16 bytes are header, the value at index 33 of the following payload is the cabinet id.

krskajo1 commented 2 months ago

Hello again. I can confirm, that the Plug is able to change the Amp to 'Studio Preamp'. Thanks! Regarding the cabinet ids, I used Fender FUSE to grab the packets and probably had the cabinet set to 'Off' when changing the amplifiers.

offa commented 2 months ago

Thanks for testing, I'm going to add the others too then.

offa commented 2 months ago

The v2 models have now been added, and they can only be selected in the UI if the appropriate device is available. Could you give the branch a test once more?

krskajo1 commented 2 months ago

Hello, all the Mustang I V2 amplifiers are now recognized and I can switch between them. Thanks a lot! I consider this issue to be solved and closed since 2 weeks ago. If you are interested into extending the support for Mustang I V2 I would create a separate "issue" for it. I got some unknown effects ID while testing and I suspect there might be some more. Thanks a lot!

offa commented 2 months ago

Yes, improving the v2 support would be nice. Feel free to open an issue for what is missing.

offa commented 2 months ago

Changes merged to master.