fix ElektronPattern implementation virtual methods, fix SDDrive read

[jmamma]

Is the constructor (bool _init = true) required?

Initialisation looks to be setting static parameters related to the type.

In other words, in what scenario are these anything but the constructor values ? maxSteps = 64; maxParams = 24; maxTracks = 16;
maxLocks = 64;

---

Also, are we certain about clearPattern() originally calling base class {}

I have some concerns that the lock arrays need to be correctly initialized.

654 void MDSeqTrack::merge_from_md(uint8_t track_number, MDPattern *pattern) {
655   DEBUG_PRINT_FN();
656   for (int i = 0; i < 24; i++) {
657     if (!IS_BIT_SET32(pattern->lockPatterns[track_number], i)) {
658       continue;
659     }
660     int8_t idx = pattern->paramLocks[track_number][i];
661     if (idx < 0) {
662       continue;
663     }
664     for (int s = 0; s < 64; s++) {
665       int8_t lockval = pattern->locks[idx][s];
666       if (lockval >= 0 &&
667           IS_BIT_SET64(pattern->trigPatterns[track_number], s)) {
668         set_track_locks(s, i, lockval);
669       }
670     }
671   }

Checks for lockval >= 0

[jmamma]

File was renamed from MDPattern.hh -> MDPattern.h, so git history only goes back to Augusst.

https://github.com/jmamma/MIDICtrl20_MegaCommand/blob/2.70/avr/cores/megacommand/MD/MDPattern.hh

[jmamma]

https://onlinegdb.com/rye27Hsj2v

^

According to this behaviour the original implementation called MDPattern::clearPattern() not base class virtual clearPatter {}

[yatli]

Also, are we certain about clearPattern() originally calling base class {}

It is called in the base method init()

[jmamma]

RIght. Doesn't explain how lock decoding was working then ?

[yatli]

You are right.. The derived method IS virtual even if it's not declared with the virtual keyword.

[yatli]

https://www.onlinegdb.com/fork/rye27Hsj2v

[yatli]

Guess I could never be a C++ pro (

[jmamma]

Yeah, i had to double check out of curiosity.

https://onlinegdb.com/H17muoonD

Type of question they throw on a CS exam.

[jmamma]

If it was multiple choice you'd have a 25% chance of being a C++ pro.

[yatli]

If it was multiple choice you'd have a 25% chance of being a C++ pro.

Coin flippin pro (flip twice)

Actually the whole XPattern : ElektronPattern thing surprise me as the base defines concrete data fields (locks etc.)

[yatli]

    uint8_t maxParams;
    uint8_t maxTracks;
    uint8_t maxSteps;
    uint8_t maxLocks;

Initialisation looks to be setting static parameters related to the type.

Depends on how generic we want them to be. Setting static members means we cannot get the correct value in a "virtualized" routine.

We can declare them as const in the base class and pass in the value to the base ctor: http://www.cplusplus.com/forum/general/11196/#msg52877

Of course this will only be useful when we are able to decode MNMPattern and A4Pattern.

[jmamma]

Still seems to be a problem here...

SDDrive restore doesnt work properly.. causes MD tempo to glitch to 247.7

[jmamma]

ClearPattern() is no longer being called.. but the restored pattern data is still broken.

[yatli]

Try soft reset first?

[jmamma]

Try soft reset first?

I've been doing factory resets and soft resets all day.

Are you able to sort this out?

Blocker for 3.0. I need to get it released tonight.

[jmamma]

The other option is that I drop SD Drive from the release.

[yatli]

Probably a good idea -- make it back when the sampling feature is ready. By then we should be able to also backup/restore the samples.

[jmamma]

KitData is empty when transmitting...

[jmamma]

  //  Kits
  progress_max = 64; 
  for (int i = 0; i < 64; ++i) {
    progress_i = i;
    mcl_gui.draw_progress("Loading kit", i, 64);
    if (!mcl_sd.read_data_v<MDKit>(&MD.kit, &file)) {
      goto load_error;
    }   
    MD.setSysexRecPos(4, i); 
    delay(20);
    DEBUG_PRINTLN(MD.kit.name); <---- Prints empty string
    MD.kit.toSysex();

[jmamma]

I made this change

   template <class T> bool read_data_v(T *data, FatFile *filep) {
     auto ret = read_data(data, sizeof(T), filep);
-    ::new(data)T();
+    ::new(data)T;
     return ret;
   }

and it seems to be fixed?

[jmamma]

Kit and Pattern data is working now.

Globals still broken..

[jmamma]

MDGlobal has a constructor whereas MDKit didn't.

[yatli]

-    ::new(data)T();
+    ::new(data)T;

I don't quite understand this (0o0) Both seem to be placement-new.

Wait, it wasn't here before in 2.7 was it? The whole placement-new thing is introduced when we were working on the generic seq track classes.

[jmamma]

The one thing I noticed was that MDPattern and MDKit did not have constructors..

I compares with deviceTrack implementation.. and it didn't use ()

[jmamma]

MDGlobal problem seems different. It could be with sysex encoding. Can you take a look?

[jmamma]

Hmm, i try MDGlobal.from and MDGlobal.to and it works okay from global 0 -> to global 1

[yatli]

How did you do that? Isn't the target slot governed by MD.setSysexRecPos(2, i); ?

[yatli]

The one thing I noticed was that MDPattern and MDKit did not have constructors..

No constructor = compiler will auto create "trivial constructor"

[jmamma]

  MD.getBlockingGlobal(i);

    MD.setSysexRecPos(2, i + 1); 
    ElektronDataToSysexEncoder encoder(&MidiUart);
      delay(20);
      MD.global.toSysex(&encoder);

[jmamma]

I think the global tempo decoding is broken.

     if (!MD.getBlockingGlobal(i)) {
      error_is_md = true;
      DEBUG_PRINTLN("error global");
      goto save_error;
    }
    DEBUG_PRINTLN("global");
    DEBUG_PRINTLN(MD.global.origPosition);
    DEBUG_PRINTLN(MD.global.tempo);

21:48:41.303 -> global
21:48:41.303 -> 0  <- orig position
21:48:41.303 -> 11832 <- tempo * 24

[jmamma]

MD.global.tempo) should == 3000 for 125bpm

[jmamma]

any idea what might have changed?

[yatli]

git blame shows I refactored it in Sep.

[jmamma]

100   origPosition = midi->midiSysex.getByte(offset + 3);
101   ElektronSysexDecoder decoder(midi, offset + 4);
102   decoder.stop7Bit();
103   decoder.get(drumRouting, 16);
104 
105   decoder.start7Bit();
106   decoder.get(keyMap, 128);
107   decoder.stop7Bit();
108 
109   decoder.get8(&baseChannel);
110   decoder.get8(&unused);
111   decoder.get16(&tempo);
112   decoder.getb(&extendedMode);
113 
114   uint8_t byte = 0;
115   decoder.get8(&byte);
116   clockIn = IS_BIT_SET(byte, 0);
117   transportIn = IS_BIT_SET(byte, 4);
118   clockOut = IS_BIT_SET(byte, 5);
119   transportOut = IS_BIT_SET(byte, 6);
120   decoder.getb(&localOn);
121 
122   decoder.get(&drumLeft, 12);

 

[yatli]

111 decoder.get16(&tempo);

There it is

[yatli]

This thing should be broken from the very start (??)

[jmamma]

Yeah, i suspect it was, but im not sure why 2.70 didnt glitch out the tempo on global transmission.

[yatli]

toSysex does it right:

  uint8_t tempo_lower = (uint8_t)(tempo & 0x7F);
  uint8_t tempo_upper = (uint8_t)(tempo >> 7);
  encoder->pack8(tempo_upper);
  encoder->pack8(tempo_lower);

[jmamma]

can you submit a patch?

i need to eat some food.

[yatli]

Can do. But no access to MD so can't test it.

[yatli]

I'll think about this

[jmamma]

we can always override tempo with a default value. tempo doesnt change when switching globals if I recall correctly.

[yatli]

Patched ^^^

[yatli]

ef5399b why 😂😂😂

[yatli]

C++ rule of thumb

• Elegant stuff doesn't work
• Working stuff ain't elegant

[jmamma]

sorry i missed a file. forced pushed eef9f12

[jmamma]

machinedrum lockup on recv global :(

[yatli]

p = new T; // default init
p = new T(); // value init -- if no ctor is provided by user, then conducts zero init (zeros everything)

See: https://en.cppreference.com/w/cpp/language/new#Placement_new https://en.cppreference.com/w/cpp/language/value_initialization https://en.cppreference.com/w/cpp/language/default_initialization

[jmamma]

if no ctor is provided by user, then conducts zero init (zeros everything) ahaha.