jmamma / MCL

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

Circular dependency MCLMemory.h #114

Closed jmamma closed 4 years ago

jmamma commented 4 years ago

1238c30e408f0a60a1eaac248e07d7ed8fc54b11

/Applications/Arduino.app/Contents/Java/hardware/valence/avr/cores/megacommand/MCL/MCLMemory.h:37:30: error: 'MDTrackLight' was not declared in this scope

define MD_TRACK_LEN (sizeof(MDTrackLight))

I refactored the ExtTrack class to inherit Bank1Object for store_in_mem() functionality.

ExtTrack

 10 
 11 class ExtTrack : public GridTrack,
 12                  public Bank1Object<ExtTrack, NUM_MD_TRACKS, BANK1_A4_TRACKS_START> {

Now compiling fails for MCLMemory.h because MDTrackLight is not declared.

MCLMemory.h

 39 #define MD_TRACK_LEN (sizeof(MDTrackLight))
 40 #define A4_TRACK_LEN (sizeof(A4Track))

MDTrack.h includes "MCLMemory.h"

jmamma commented 4 years ago

@yatli do you have an spare CPU cycles to help me troubleshoot this one?

I can't think of a clever way to get around the fact that MDTrack.h and MCLMemory.h both need to include each other

forward declaration of MDTrackLight wont work because sizeof() needs a complete type.

yatli commented 4 years ago

Can we split a part of MCLMemory.h into a separate header? MDTrack.h can include that one.

jmamma commented 4 years ago

I tried moving BANK1_MD_TRACK_START to MDTrack.h (where it is used) and include "MDTrack.h" in MCLMemory.h

but now A4Track.h is complaining it doesn't have the declaartion for BANK1_A4_TRACKS_START (which is in MCLMemory.h)

51 #define BANK1_A4_TRACKS_START (BANK1_MD_TRACKS_START + MD_TRACK_LEN * NUM_MD_TRACKS)

🤯

yatli commented 4 years ago

Does A4Track.h include MCLMemory?

jmamma commented 4 years ago

yes

A4Track.h:

  5 
  6 #include "ExtTrack.h"
  7 // include full MDTrack specification to calculate size
  8 #include "MDTrack.h"
  9 #include "A4.h"
 10 #include "Project.h"
 11 #include "MCLMemory.h"
 12 #include "Bank1Object.h"
 13 
 14 class A4Track : public GridTrack,
 15                 public Bank1Object<A4Track, NUM_MD_TRACKS, BANK1_A4_TRACKS_START> {

MDTrack.h currently:

  3 #define BANK1_MD_TRACKS_START (BANK1_SYSEX2_DATA_START + SYSEX2_DATA_LEN)
  4 
  5 #ifndef MDTRACK_H__
  6 #define MDTRACK_H__
  7 
  8 // 16x MD tracks
  9 

MCLMemory.h currently:

36 #include "MDTrack.h"
 37     
 38 #define MD_TRACK_LEN (sizeof(MDTrackLight))
 39 #define A4_TRACK_LEN (sizeof(A4Track))
 40   
 41 #ifdef EXT_TRACKS
 42 #define EMPTY_TRACK_LEN A4_TRACK_LEN
 43 #else
 44 #pragma message("EMPTY_TRACK_LEN == MD_TRACK_LEN")
 45 #define EMPTY_TRACK_LEN MD_TRACK_LEN
 46 #endif
 47 
 48 // 16x MD tracks
 49 //#define BANK1_MD_TRACKS_START (BANK1_SYSEX2_DATA_START + SYSEX2_DATA_LEN)
 50 // 4x A4 tracks
 51 #define BANK1_A4_TRACKS_START (BANK1_MD_TRACKS_START + MD_TRACK_LEN * NUM_MD_TRACKS)
yatli commented 4 years ago
MDTrack.h [origin] [defines MD_TRACK_LEN, BANK1_MD_TRACKS_START]
A4Track.h [includes MDTrack.h] [defines A4_TRACK_LEN, BANK1_A4_TRACKS_START]
MCLMemory.h [includes MDTrack.h A4Track.h]

Does this solve the problem?

yatli commented 4 years ago

Hmm, do we need MCLMemory.h at all? Edit: ok need it for file entry positions. So it's reasonable it requires the proper definition of MDTrack and A4Track positions.

jmamma commented 4 years ago

Does this solve the problem?

Doesn't work. MD_TRACK_LEN needs to be defined in MDTrack.h

But A4Track.h doesnt see it. /Applications/Arduino.app/Contents/Java/hardware/valence/avr/cores/megacommand/MCL/A4Track.h:14:56: error: 'MD_TRACK_LEN' was not declared in this scope

I think because of ifndef.

  6 #ifndef MDTRACK_H__
  7 #define MDTRACK_H__
...
 58 class MDTrackLight
 59     : public GridTrack,
 60       public Bank1Object<MDTrackLight, 0, BANK1_MD_TRACKS_START> {
 61 public:
 62   MDSeqTrackData seq_data;
 63   MDMachine machine;
 64 };
 65 
 66 
 67 #define MD_TRACK_LEN (sizeof(MDTrackLight))
yatli commented 4 years ago

Does A4Track.h include MDTrack.h?

jmamma commented 4 years ago

yeah...

yatli commented 4 years ago

If the ifndef guard hides the range, it's because it's already included. So it's impossible that the condition goes true and the requiring source doesn't see the content.

yatli commented 4 years ago

Try replace the #define with constexpr size_t MD_TRACK_LEN=sizeof(MDTrackLight);?

jmamma commented 4 years ago

Reset the branch to 1238c30

/Applications/Arduino.app/Contents/Java/hardware/valence/avr/cores/megacommand/MCL/MCLMemory.h:37:38: error: 'MDTrackLight' was not declared in this scope constexpr size_t MD_TRACK_LEN=sizeof(MDTrackLight);

jmamma commented 4 years ago

oh right you were suggesting add it in A4Track.h ?

Getting late here... might pick this up in the morning.

jmamma commented 4 years ago

Found a hacky way around this..

you had these two defines commented out.

 26 #include "MDSeqTrack.h"
 27 #include "ExtSeqTrack.h"
 28 #include "MD.h"
 29 
 30 #define MD_TRACK_LEN (sizeof(GridTrack) + sizeof(MDSeqTrackData) + sizeof(MDMachine))
 31 #define A4_TRACK_LEN (sizeof(GridTrack) + sizeof(ExtSeqTrackData) + sizeof(A4Sound))
jmamma commented 4 years ago

Okay... compiles.

MCLActions refactored, and stack overflow avoided. Code looks much cleaner now.