Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arpeggiator seems to not function on device #26

Open
jaygarcia opened this issue Dec 7, 2017 · 18 comments
Open

Arpeggiator seems to not function on device #26

jaygarcia opened this issue Dec 7, 2017 · 18 comments
Assignees

Comments

@jaygarcia
Copy link

jaygarcia commented Dec 7, 2017

Arpeggiator seems not function in the current Master. It plays once through and it's done.

Seems to work in the UI. Can't tell where the problem is.

(updated to attach files)

@jaygarcia
Copy link
Author

arpeggiator.atm.txt

@jaygarcia
Copy link
Author

arpeggiator.h.txt

@dxxb
Copy link

dxxb commented Dec 7, 2017

New docs:

P2
    Size   : 1 byte
    Name   : Effect configuration
    Format : b-edttttt
              |||└└└└└-> ticks between note change minus one [0, 31]
              ||└------> 0: auto repeat, 1: trigger with note
              |└-------> 1: skip 3rd note, 0: play 3rd note
              └--------> [reserved]

Old docs (these are only partially visible on the page because of markup errors, you need to open the raw view to read the complete content of the cells):

Next to the current playing note, play a second
and third note *[__X__]* for every *[__Y__]* ticks.

*[__X__]* includes 2 parameters: AAAABBBB, where
AAAA = base + amount to second note and
BBBB = second note + amount to third note.

*[__Y__]* includes 4 parameters: FEDttttt,
where F = reserved, E = toggle no third note,
D = toggle retrigger
ttttt = tick amount.

**_Note:_** Arpeggio is used for playing 3 notes out of a chord indivually

The offending song uses ATM_CMD_M_ARPEGGIO_ON(221, 97) which means 0xdd, 0x61, first byte is the notes offset in semitones so 2nd and 3rd note are 13 semitones up from the base note. Second byte says to play a new note every 2 ticks and don't play the 3rd note. Which means it will alternate between base_note and base_note+13 semitones. Second byte also sets bit 5. bit 5 is documented in the old docs as "toggle retrigger" (it doesn't say exactly what bit states 0 and 1 mean nor what exactly retrigger means).

The original code restarts the arpeggio on note-ON when bit 5 is set but lets the arpeggio continue until stopped explicitly independently of bit 5.

The new code plays a chord on note-ON when bit 5 is set (like the old code) but when bit 5 is reset it plays the chord once i.e. automatic chord re-trigger is disabled.

My reasoning was as follows: The docs were not clear in explaining how the effect should work so I looked at the code and saw that the chord was restarted when bit 5 was set but when bit 5 was reset it was ignored resulting in the behaviour outlined above. I figured that, as implemented, the bit wasn't useful because the original behaviour can be implemented without using a dedicated bit at all. So I concluded that there was probably a bug and the intention was to differentiate between chords that play once on every note-ON and chords that keep playing and that's what I have implemented in the current library.

We can fix this in at least two ways:

  1. Change the UI to state more clearly what the tick box means (e.g. "auto re-trigger" vs "note triggered") and set or reset bit 5 according to the new implementation.

  2. Change the library to swap the meaning of 0 and 1 for bit 5 and state more clearly what the tick box means (e.g. "auto re-trigger" vs "note triggered").

Comments?

@jaygarcia
Copy link
Author

@dxxb thank you very much for this.

I'm OK w/ changing the UI, but that also means digging into ATMLib.js and I've got zero bandwidth at this time.

@slemmon please work to update the UI and I'll try to find time to update the JS Synth in 2018.

@dxxb
Copy link

dxxb commented Dec 7, 2017

For a quick fix change:

ATM_CMD_M_ARPEGGIO_ON(221, 97), \

to:

ATM_CMD_M_ARPEGGIO_ON(221, 65), \

it will change the arpeggio to be continuous in your song. Tell me if/how I can help you further.

@jaygarcia
Copy link
Author

@slemmon can you get this to work properly in ShowCodeNew tomorrow?

I'm going to need to heavily rely on arpeggiator to reduce music size.

@slemmon
Copy link

slemmon commented Dec 8, 2017

PR: #37

@jaygarcia I'm not 1000% sure I follow yet how this all will work but I think for the moment this change will allow the UI to keep working like it's been working and export the arpeggio command expected by lib2 for player playback.

If I've missed the mark I'll rework it tomorrow once you, me, and dxxb are online.

@dxxb
Copy link

dxxb commented Dec 8, 2017

I'm going to need to heavily rely on arpeggiator to reduce music size.

FYI, the arpeggiator also supports alternating between 1 note and silence (equally spaced), see notecut. Mentioning it just in case.

PR: #37

I see you changed the value assembled by the UI. That's one way to do it. I think changing the text associated with that FX option is essential to spare confusion on the user part. You could even get away with leaving the code as is with the right wording in the UI.

@jaygarcia
Copy link
Author

jaygarcia commented Dec 9, 2017

@slemmon it doesn't work at all. Have you tested on device? Try the below atm file for something that is sonically more pleasing.
arpeggiator2.atm.txt

@dxxb
Copy link

dxxb commented Dec 9, 2017

@jaygarcia, could it be a problem with the library instead? I don't have an arduboy with me right now so I cannot test it myself. Does changing manually:

ATM_CMD_M_ARPEGGIO_ON(221, 97), \

to:

ATM_CMD_M_ARPEGGIO_ON(221, 65), \

solve the issue?

@jaygarcia
Copy link
Author

@dxxb i'm working on that now. I am not good w/ numbers to binary so I am working on it by hand.

ATM_CMD_M_ARPEGGIO_ON(0b00110100, 0b01000001),

@jaygarcia
Copy link
Author

@dxxb yeah it's an issue with the library (i think).

ATM_CMD_M_ARPEGGIO_ON(221, 65), \ <-- Still fails

@jaygarcia
Copy link
Author

@dxxb here is the header file
arpeggiator.h.txt

@dxxb
Copy link

dxxb commented Dec 9, 2017

I just noticed this ATM_CMD_M_SET_TEMPO(5) tempo below 8 is not supported moduscreate/ATMlib2#15 :(

@jaygarcia
Copy link
Author

Thanks @dxxb

I changed that block to the following:

My interpretation:
(Arg 1)

  • 2nd note shift 3 semitones 0011
  • 3rd note shift 4 semitones 0100

(Arg 2) 0b00000001

  • 1 Tick between note changes
  • Auto repeat on
  • play 3rd note on

/* pattern (channel) / bytes = 10*/
#define arpeggiator_pattern0_data { \
    ATM_CMD_M_SLIDE_VOL_ON(110), \
    ATM_CMD_M_ARPEGGIO_ON(0b00110100, 0b00000001), \
    ATM_CMD_M_SET_TEMPO(20), \
    ATM_CMD_M_CALL(4), \
    ATM_CMD_M_SET_LOOP_PATTERN(0), \
    ATM_CMD_I_STOP, \
}

Docs for reference :D

Arpeggio - Play a second and optionally third note after each played note

Macros         : ATM_CMD_M_ARPEGGIO_ON(p1, p2)
               : ATM_CMD_I_ARPEGGIO_OFF

Parameter count: 2

P1
    Size   : 1 byte
    Name   : Chord note shifts
    Format : bkkkknnnn
              ||||└└└└-> 3nd note shift: semitones to add to the 2nd [0,14]
              └└└└-----> 2nd note shift: semitones to add to the 1st [0,14]

P2
    Size   : 1 byte
    Name   : Effect configuration
    Format : b-edttttt
              |||└└└└└-> ticks between note change minus one [0, 31]
              ||└------> 0: auto repeat, 1: trigger with note
              |└-------> 1: skip 3rd note, 0: play 3rd note
              └--------> [reserved]

@dxxb
Copy link

dxxb commented Dec 9, 2017

I just noticed this ATM_CMD_M_SET_TEMPO(5) tempo below 8 is not supported moduscreate/ATMlib2#15 :(

Even so, you should be able to hear something. The pattern should alternate between two notes (the 3rd note is disabled when bit 6 is 0). Notes should be 2 ticks apart so: note-on, delay 2 ticks, 2nd note, delay 2 tick, 1st note.

@dxxb
Copy link

dxxb commented Dec 9, 2017

ticks between note change minus one setting ticks to 1 means 2 ticks i.e. every other tick.

@jaygarcia
Copy link
Author

Thanks. Got this working but am going to do it manually until we get the UI to do what it needs to do. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants