amowry / WARBL2

WARBL2 Code and design files
https://warbl.xyz/index.html
GNU General Public License v3.0
11 stars 3 forks source link

Channel number mask #12

Closed ithinu closed 5 months ago

ithinu commented 5 months ago

In Functions.ino:3186 c &= 0xF assumes channel = 0 ... 15 but arduino_midi_library/MIDI.hpp:732 (inChannel - 1) & 0x0f suggests that channel = 1 ... 16. I did not check if you use that exact library, but maybe worth looking into.

amowry commented 5 months ago

Thanks, I think you're right-- I thought the default channel for messages was 0, but it looks like it's actually 1 and it's currently only possible to send messages on channels 1-15. I think the source of my confusion was that the Celtic Sounds app, which a lot of WARBL owners use, has the same mistake, i.e. the lowest channel allowed is channel 1, not channel 0. I'll need to think about how to fix this without making it confusing for users. I may extend the range to include channel 0 but leave the default channel at 1.

essej commented 5 months ago

No, there is no "channel 0" with our current use of channel numbers. The binary representation of midi messages themselves use 0-15 of course, but all the MIDI apis used in the software represent the channels as 1-16, so we should continue that. In our sendMIDI() at https://github.com/amowry/WARBL2/blob/cfa142b93cbac3ba4432ce4b904d11ee4fb64860/warbl2_firmware/Functions.ino#L3204 the channel parameter (c) is sent in as 1-16 as well, and we just need to change that mask to a c = constrain(c, 1, 16);

amowry commented 5 months ago

No, there is no "channel 0" with our current use of channel numbers.

So, you're saying that we're just ignoring channel 0, is that correct? I guess I've always had a fundamental misunderstanding about this. I thought that we were sending on channel 0 by default but calling it channel 1, and that most MIDI apps that allow channels "1-16" are actually using 0-15.

I'm realizing now that MIDI apps typically just don't use channel 0, is that correct? So we don't need to include the ability to send on channel 0?

essej commented 5 months ago

This is MIDI's fault that it is confusing, it's just a naming convention thing. There are only sixteen channels, and usually they are named for humans 1-16. Most API's including the one we are using ALSO take channel as a number from 1-16. The low-level byte protocol itself however represents channels in the status message as 0-indexed, but we don't operate at that level in this codebase. So, henceforth, in all places where you present a midi channel number just continue to use 1-16 as the allowable range, because that won't break anything, and is correct in the context we operate in right now!

seisiuneer commented 5 months ago

In Celtic Sounds, my slider goes from 1-16, but I'm pretty sure that maps to 0-15

On Mon, Jul 1, 2024 at 11:15 AM Jesse Chappell @.***> wrote:

This is MIDI's fault that it is confusing, it's just a naming convention thing. There are only sixteen channels, and usually they are named for humans 1-16. Most API's including the one we are using ALSO take channel as a number from 1-16. The low-level byte protocol itself however represents channels in the status message as 0-indexed, but we don't operate at that level in this codebase. So, henceforth, in all places where you present a midi channel number just continue to use 1-16 as the allowable range, because that won't break anything, and is correct in the context we operate in right now!

— Reply to this email directly, view it on GitHub https://github.com/amowry/WARBL2/issues/12#issuecomment-2200747004, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4ZXL44K3HDBYRK557YKVTZKGMCPAVCNFSM6AAAAABKF3HDPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBQG42DOMBQGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--


Michael Eskin @.*** (619) 368-1854 (text/voice)

Home Page http://michaeleskin.com | ABC Tools https://michaeleskin.com/abc | Zoom Session http://michaeleskin.com/session | AppCordions http://appcordions.com | Flickr https://www.flickr.com/photos/eskin/albums


seisiuneer commented 5 months ago

I could be mistaken.

On Mon, Jul 1, 2024 at 11:17 AM Michael Eskin @.***> wrote:

In Celtic Sounds, my slider goes from 1-16, but I'm pretty sure that maps to 0-15

On Mon, Jul 1, 2024 at 11:15 AM Jesse Chappell @.***> wrote:

This is MIDI's fault that it is confusing, it's just a naming convention thing. There are only sixteen channels, and usually they are named for humans 1-16. Most API's including the one we are using ALSO take channel as a number from 1-16. The low-level byte protocol itself however represents channels in the status message as 0-indexed, but we don't operate at that level in this codebase. So, henceforth, in all places where you present a midi channel number just continue to use 1-16 as the allowable range, because that won't break anything, and is correct in the context we operate in right now!

— Reply to this email directly, view it on GitHub https://github.com/amowry/WARBL2/issues/12#issuecomment-2200747004, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4ZXL44K3HDBYRK557YKVTZKGMCPAVCNFSM6AAAAABKF3HDPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBQG42DOMBQGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--


Michael Eskin @.*** (619) 368-1854 (text/voice)

Home Page http://michaeleskin.com | ABC Tools https://michaeleskin.com/abc | Zoom Session http://michaeleskin.com/session | AppCordions http://appcordions.com | Flickr https://www.flickr.com/photos/eskin/albums


--


Michael Eskin @.*** (619) 368-1854 (text/voice)

Home Page http://michaeleskin.com | ABC Tools https://michaeleskin.com/abc | Zoom Session http://michaeleskin.com/session | AppCordions http://appcordions.com | Flickr https://www.flickr.com/photos/eskin/albums


essej commented 5 months ago

It's just a context thing. If we were constructing the bytes that go into a midi message, we'd need to do something 0-indexed, but since we are just using other API calls that take 1-indexed channel numbers also, we don't.

amowry commented 5 months ago

Yes, sorry, I confused myself on this earlier-- you're both right, and nothing is wrong with Celtic Sounds :). The only issue was that the WARBL wouldn't ever send on channel 16, which is fixed now.

MrMep commented 5 months ago

Just to be pedantic, but I guess this is technically wrong too:

https://github.com/amowry/WARBL2/blob/ad7f9d3687b0f1574e1e127ddef66c8111226464/warbl2_firmware/Functions.ino#L1598

The Config tool channel is hardcoded, so it's not a problem at all, unless we switch to channel 16 :)

amowry commented 5 months ago

Right, thanks!

joebowbeer commented 5 months ago

Just to be pedantic, but I guess this is technically wrong too:

https://github.com/amowry/WARBL2/blob/ad7f9d3687b0f1574e1e127ddef66c8111226464/warbl2_firmware/Functions.ino#L1598

The Config tool channel is hardcoded, so it's not a problem at all, unless we switch to channel 16 :)

I think this is more than pedantry.

channel should be constrained to be between 1..16 right?

The 0x0f mask is not appropriate here

amowry commented 5 months ago

Yes, I removed the mask. It's been in there forever and I'm not sure why :)