Pirate-MIDI / Pirate-MIDI-BridgeOS

Documenting Bugs & Issues - Public Collaboration
10 stars 2 forks source link

Editor: Scrolling Messages are not persisted when sending to device or exporting to file. #158

Closed TheOcelot42 closed 1 year ago

TheOcelot42 commented 1 year ago

Device(please complete the following information):

Describe the bug A "scrolling" foot switch cannot be triggered by its secondary action.

To Reproduce Steps to reproduce the behavior:

  1. Factory reset device
  2. Install FW v1.2.2.
  3. Install the attached json file.

Expected behavior The secondary action should trigger the message in the "Scrolling" message stack.

Screenshots

2x-Momentary 2x-Toggle 2x-Toggle2 Hold_Toggle2 Hold-Toggle Screen Shot 2023-03-05 at 17 38 47 Send-Hold Send-Hold2

ScrollSecondarySend_Broken_SeqSecondarySendOK_v_1.2.2_2023-03-05_17-32-18.json.zip

TheOcelot42 commented 1 year ago

Per Sam factory reset the device under test. Installed the attached json file.

ScrollingSecondaryActionDoesn'tWork.json.zip

TheOcelot42 commented 1 year ago

Work around:

Edit the scrolling message list on the hardware directly. This issue seems to be rooted in the editor not the device firmware.

simonaglover95 commented 1 year ago

Confirmed all secondary modes working when setting up onboard the device.

Confirmed wed editor not sending scrolling messages.

konnsim commented 1 year ago

App is sending the correct thing to the device, here's the bank0 DTXR ripped straight from logs based on the "ScrollingSecondaryActionDoesn'tWork.json" config above. bank0-extracted-from-send.zip

It looks correctly formed to me based on API specs (image is of example API packet) image

I can also export a config from the app with scrolling messages and then import the file and they are all there (attached file is export after importing ScrollingSecondaryActionDoesn'tWork.json) Bridge 6_2023-03-16_21-46-57.zip

To be clear @TheOcelot42 , you are definitely having issues both sending to the device and exporting to file? If so what is wrong with the config when you analyse/re-import the file?

TheOcelot42 commented 1 year ago

Hello @Konnsalad :

I was 100% having issues with messages being stripped when sent from the editor to the device. When re-imported into the editor from the DUT, the scrolling messages were (unsurprisingly) missing. @simonaglover95 appears to have done some investigation and concurs at least in part.

It doesn't appear that I captured the app version I was using when I filed the bug; is it possible that has changed in the tooling, and this problem is no longer reproducible?

How can I be more helpful? I am quite happy to do any leg work needed to either confirm that this is fixed or to further triage.

--CMP

konnsim commented 1 year ago

Hi @TheOcelot42 I think I've misunderstood the heading that you have on this issue.

So you are definitely experiencing an issue where you configure scrolling messages in the editor, send that to your device, and the config that is applied to the device doesn't include these messages. Based on the investigation I did yesterday that looks like a device firmware API parsing issue, @samspencer5991 can confirm though.

The part I'm not clear on is the "or exporting to file" part of your issue, do you mean to say that you followed the process: configure scrolling messages in app -> export as file (so no interaction with the device) and the file does not contain the scrolling messages? if this is the case then I need to do some further investigation.

or are you saying that you followed the process: configure scrolling messages in app -> send to device -> import from device -> export as file and the file does not contain the scrolling messages? If this is the case then the issue is only with the device's parsing of config.

FYI there hasn't been any app updates that could impact this behaviour since you posted the issue.

samspencer5991 commented 1 year ago

@TheOcelot42 a similar bug report was posted as well and I believe I've tracked down and fixed the cause of this. Stay tuned for a new firmware release this week.

TheOcelot42 commented 1 year ago

Awesome. Thanks for the update. I really appreciate the investigation. :)