jmamma / MCL

MCL firmware for the MegaCommand MIDI Controller.
BSD 2-Clause "Simplified" License
48 stars 9 forks source link

A4Sound conversion 2.70 -> 3.0 #141

Closed jmamma closed 3 years ago

jmamma commented 3 years ago

Currently sound conversion is not working.

yatli commented 3 years ago

@jmamma is master 2.7? I'd like to do some A/B testing.

yatli commented 3 years ago

193961226bf35c791f1287448994c63a768df14f fixed sound conversion on my side.

jmamma commented 3 years ago

Yes master is 2.7

I will test this soon.

jmamma commented 3 years ago

Yes looking good!

Thankyou :rocket:

jmamma commented 3 years ago

Didnt take long to find another bug... midi channel becomes corrupt during transition_load. okay during load_immediate.

time for dinner.

yatli commented 3 years ago

Can confirm. Chaining while playback result in corrupted sound/sequence.

yatli commented 3 years ago

The corruption is only triggered when M5 and M6 are involved?

A4: I told you my name is Analog FOUR

jmamma commented 3 years ago

Hmm. This might be a different problem. In my converted project only tracks 1-4 are active.

jmamma commented 3 years ago

Ah I think you're right. tracks 5 +6 get loaded anyway.

Thought I fixed this!!

yatli commented 3 years ago

Yeah, could you try to chain all the tracks with TI? I have problem with "chain group", but if I select the 4 tracks with TI it's fine

jmamma commented 3 years ago
 34 void A4Class::init_grid_devices() {
 35   uint8_t grid_idx = 1;
 36 
 37   for (uint8_t i = 0; i < NUM_EXT_TRACKS; i++) {
 38     uint8_t track_type= EXT_TRACK_TYPE;
 39 
 40     if (i < NUM_A4_SOUND_TRACKS) {
 41        track_type = A4_TRACK_TYPE;
 42     }
 43 
 44     add_track_to_grid(grid_idx, i,  &(mcl_seq.ext_tracks[i]), track_type);
 45   }
 46 
 47 }

I did patch the device/slot allocation.

The problem must be with Chain mode loading the track as A4 instead of EXT

yatli commented 3 years ago

Very likely...

A4: image

jmamma commented 3 years ago

no more bugs plz.

jmamma commented 3 years ago
transition_load_ext
34130 <- seq ptr
17 <- slot number
1 <-track number
here
send a4 sound  <--- A4 Tracks transmit sound.
transition_load_ext
36138
18
2
here
send a4 sound
transition_load_ext
38146
19
3
transition_load_ext 
40154
20
4
transition_load_ext
42162
21
5

Appears to be loading correctly during transition. or at least the correct track type

jmamma commented 3 years ago

actually that is misleading... send_machine[n] determines if sound data is sent. A4Track:transition_load calls ExtTrack::transition_load

jmamma commented 3 years ago

i.e

134       auto *pmem_track = empty_track.load_from_mem(track_idx, track_type);
135 
136       if (pmem_track != nullptr) {
137         if (mcl_actions.send_machine[n] == 0) {
138         pmem_track->transition_send(track_idx, n);
139         }
140         pmem_track->transition_load(track_idx, seq_track, n);
141         grid_page.active_slots[n] = slots_changed[n];
142       }
143 
jmamma commented 3 years ago
291   for (uint8_t n = 0; n < NUM_SLOTS; ++n) {
292 
293     SeqTrack *seq_track =
294         get_dev_slot_info(n, &grid_idx, &track_idx, &track_type, &dev_idx);
295     proj.select_grid(grid_idx);
296     if ((slot_select_array[n] == 0) || (track_type == 255)) {
297       // Ignore slots that are not device supported.
298       slot_select_array[n] = 0;
299       continue;
300     }
301     auto device_track = empty_track.load_from_grid(track_idx, row);
302     if (device_track == nullptr || device_track->active != track_type) {
303       empty_track.clear();
304       device_track = device_track->init_track_type(track_type);
305       send_machine[n] = 1;
306     } else {
307       send_machine[n] = 0;
308     }
309     DEBUG_PRINTLN("track type load");
310     DEBUG_DUMP(n);
311     DEBUG_DUMP(track_idx);
312     DEBUG_DUMP(track_type);
313     device_track->store_in_mem(track_idx);
314   }
track type load
18
2
5
2094
track type load
19
3
5 <- A4_TRACK_TYPE
2094
track type load
20
4
6
2094
track type load
21
5
6 <- EXT_TRACK_TYPE
jmamma commented 3 years ago

device_track->active = 6 as well... hmm

jmamma commented 3 years ago

gridtask loads the track from mem as EXT_TRACK_TYPE.

jmamma commented 3 years ago

That's weird. It's only broken if 4 + 5 are transmitted together. If I transmit 4 and then transmit 5 it's okay.

Transmit them together and SOUND4 appears as SOUND1 on the A4.

jmamma commented 3 years ago

get_region() missing ??

jmamma commented 3 years ago

./ExtTrack.h: virtual uint32_t get_region() { return BANK1_A4_TRACKS_START; }

hmm nope.

jmamma commented 3 years ago

76 constexpr size_t BANK1_A4_TRACKS_START = BANK1_AUX_TRACKS_START + FX_TRACK_LEN * NUM_FX_TRACKS;

this is wrong. but i dont think it's related to this issue.

56 constexpr size_t FX_TRACK_LEN = 43;

is too small. MDLFOTrack is the largest type..

jmamma commented 3 years ago

also

./MCLMemory.h:constexpr size_t NUM_LFO_TRACKS = 2;

should be 1

jmamma commented 3 years ago

We should probably create BANK1 buckets for each of the AUXTrack types.

jmamma commented 3 years ago

🛌

jmamma commented 3 years ago

constexpr size_t GRID2_TRACK_LEN = 2094;

I think this is too small ? @yatli can you check these

jmamma commented 3 years ago

KWelcome to MegaCommand Live 3000 534 2094 1754 2094

yatli commented 3 years ago

Sire, I'll check it tomorrow.

yatli commented 3 years ago

These guards ain't for nothing: static_assert(sizeof(A4Track) <= GRID2_TRACK_LEN);

jmamma commented 3 years ago
76 constexpr size_t BANK1_A4_TRACKS_START = BANK1_AUX_TRACKS_START + FX_TRACK_LEN * NUM_FX_TRACKS;

this is wrong. but i dont think it's related to this issue.

56 constexpr size_t FX_TRACK_LEN = 43;

is too small. MDLFOTrack is the largest type..

Need to fix these up.

The original bug feels like an overflow... but haven't detected the cause yet.

jmamma commented 3 years ago

Think I found it...

314 device_track->store_in_mem(track_idx);

track_idx = 5 or 6

This is a new region. so they should be 0, 1

jmamma commented 3 years ago

no wait. this is wrong. track_idx is okay for ExtTrack.

need to check track_idx for the AUXTracks.

jmamma commented 3 years ago
 129   add_track_to_grid(grid_idx, MDFX_TRACK_NUM, &(mcl_seq.aux_tracks[0]), MDFX_TRACK_TYPE, GROUP_AUX);
 130   add_track_to_grid(grid_idx, MDROUTE_TRACK_NUM, &(mcl_seq.aux_tracks[1]), MDROUTE_TRACK_TYPE, GROUP_AUX);
 131   add_track_to_grid(grid_idx, MDLFO_TRACK_NUM,  &(mcl_seq.aux_tracks[2]), MDLFO_TRACK_TYPE, GROUP_AUX);
 132   add_track_to_grid(grid_idx, MDTEMPO_TRACK_NUM, &(mcl_seq.aux_tracks[3]), MDTEMPO_TRACK_TYPE, GROUP_TEMPO);
 133 }
./Elektron.h:  void add_track_to_grid(uint8_t grid_idx, uint8_t track_idx, SeqTrack *seq_track, uint8_t track_type, uint8_t group_type = GROUP_DEV) {
 34 constexpr size_t NUM_FX_TRACKS = 2; <--- He's wrong.
 35 constexpr size_t MDFX_TRACK_NUM = 12; //position of MDFX track in grid
 36 constexpr size_t MDLFO_TRACK_NUM = 13; //position of MDLFO track in grid
 37 constexpr size_t MDROUTE_TRACK_NUM = 14; //position of MDROUTE track in grid
 38 constexpr size_t MDTEMPO_TRACK_NUM = 15; //position of MDTEMPO track in grid
jmamma commented 3 years ago

yep track_idx is wrong for

device_track->store_in_mem(track_idx);

jmamma commented 3 years ago

now how to fix this mess 🤔

yatli commented 3 years ago

I was looking at these lines and wondering what is an AUX track and what FX.

jmamma commented 3 years ago

Well I only had 1 FX track to begin with, the other 3 tracks came later. So I renamed the new group to "AUX (auxiliary) tracks"

There's a parent device class for them now. AUXTrack.h

jmamma commented 3 years ago

7a493c3d0c3b6920db451fd434a4ba313146097d

slot_mem_idx will now be used. ie:

device_track->store_in_mem(slot_mem_idx); device_track->load_from_mem(slot_mem_idx);

jmamma commented 3 years ago

More work needs to be done.

MCLActions::get_dev_slot_info(uint8_t slot_number, uint8_t *grid_idx,
 89                                         uint8_t *track_idx, uint8_t *track_type,
 90                                         uint8_t *dev_idx, uint8_t *group_type = nullptr)

Needs to be turned into:

MCLActions::get_dev_slot_info(uint8_t slot_number, uint8_t *grid_idx,
 89                                         uint8_t *track_idx, uint8_t *track_type,
 90                                         uint8_t *dev_idx, uint8_t *mem_slot_idx, uint8_t *group_type = nullptr)

But now it's really ugly.

uint8_t grid_idx, track_idx, track_type, dev_idx, mem_slot_idx, group_type;

663       SeqTrack *seq_track =
664           get_dev_slot_info(n, &grid_idx, &track_idx, &track_type, &dev_idx, &mem_slot_idx, &group_type);
jmamma commented 3 years ago

Refactor?

GridDeviceTrack *p = get_dev_slot_info(n, &grid_idx, &dev_Idx);

SeqTrack *seq_track = p->seq_track;
track_idx = p->track_idx
etc...
yatli commented 3 years ago
class DeviceSlotInfo {
public:
  uint8_t grid_idx;
  uint8_t track_idx;
  uint8_t track_type;
  uint8_t dev_idx;
  uint8_t group_type;
  SeqTrack* seq_track;
};

bool get_dev_slot_info(uint8_t slot_number, DeviceSlotInfo* pinfo);

// example:

  for (i = 0; i < NUM_SLOTS; i++) {
    if (slot_select_array[i] > 0 && get_dev_slot_info(i, &dev_info)) {
      save_dev_tracks[dev_idx] = true;
    }
  }
jmamma commented 3 years ago

i'm working through the code now.

e.g:

    GridDeviceTrack *gdt = get_grid_dev_track(i, &track_idx, &dev_idx);
    uint8_t grid_idx = get_grid_idx(i);
    proj.select_grid(grid_idx);

    if ((slot_select_array[n] == 0) || (gdt != nullptr)) {

    if (device_track == nullptr || device_track->active != gdt->track_type) {
      empty_track.clear();
      device_track = device_track->init_track_type(gdt->track_type);
      send_machine[n] = 1;
    } else {
    }
    device_track->store_in_mem(gdt->mem_slot_idx);
yatli commented 3 years ago

I'll push mine to another branch for your review.

yatli commented 3 years ago

https://github.com/jmamma/MIDICtrl20_MegaCommand/pull/145

jmamma commented 3 years ago

shit.

jmamma commented 3 years ago
224       midi_active_peering.get_device(UART2_PORT)->asElektronDevice(),
225   å;
226   if ((mcl_cfg.chain_mode > 0) && (MidiClock.state == 2)) ä
227     for (uint8_t i = 0; i < NUM_DEVS; ++i) ä
228       if (elektron_devsÄiÅ != nullptr &&
229           elektron_devsÄiÅ->canReadWorkspaceKit()) ä
230         auto kit = elektron_devsÄiÅ->getKit();
231         if (kit != nullptr &&
232             elektron_devsÄiÅ->currentKit != kit->getPosition()) ä
233           elektron_devsÄiÅ->getBlockingKit(0x7F);
234         å
235       å
236     å
237     prepare_next_chain(row, slot_select_array);
238     return;
239   å
240   for (uint8_t i = 0; i < NUM_DEVS; ++i) ä
241     if (elektron_devsÄiÅ != nullptr &&
242         elektron_devsÄiÅ->canReadWorkspaceKit()) ä
243       elektron_devsÄiÅ->getBlockingKit(0x7F);
244     å
245   å

my vim just destroyed all my changes.

jmamma commented 3 years ago

F@#$@$@#$@#$

jmamma commented 3 years ago

hmm. i think it's just the editor being broken. file is still okay 🤞

jmamma commented 3 years ago

@yatli I looked at your implementation.

I think mine will use less progmem in the end ?

1539ad6