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

Support for midi output using ALSA #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

caropf
Copy link

@caropf caropf commented Apr 23, 2020

also fixes #61

@cancel
Copy link
Collaborator

cancel commented Apr 23, 2020

This looks cool. But there are a few problems.

This mixes tab characters into the source code. It also includes non-English comments and text. It also makes some stealthy edits and changes that it shouldn't. You change the target arch from nehalem to native, ignoring the detailed and important comment above but also leaving the comment in place, making it a contradictory comment. And you don't mention this change anywhere or provide a justification for it.

But it does a good job of following the existing orca-c code and adding the ALSA MIDI feature in an appropriate way.

I'm not sure I want to add this to the Hundredrabbits orca-c right now, but I encourage you to keep your own fork of it with these changes. I don't have any way to test this myself. I could be convinced otherwise if other people think it would be a good idea, though.

@cancel
Copy link
Collaborator

cancel commented Apr 23, 2020

Maybe @neauoire and @npisanti can off their comments? Do you guys know if adding the ALSA stuff would be worth it?

@npisanti
Copy link
Collaborator

I thinks this will be a nice thing to have, as there is not that much code added and it could be good to have an alternative to portmidi. The commit surely need some cleaning, due to the fact that are included two commits that would have required different PR.

The commit for fixing the linking error in @caropf machine introduces a linking error on mine, as i don't have the tinfow installed and it is not required for building orca-c on my distro ( Debian 10 ).

The commit for the ALSA output needs some casting ( output of ./tool build --alsa orca )

tui_main.c: In functionsend_midi_3bytes’:
tui_main.c:1041:2: warning: conversion frominttounsigned charmay change value [-Wconversion]
  snd_seq_ev_set_source(&ev, midi_mode->alsa.port_id);
  ^~~~~~~~~~~~~~~~~~~~~
tui_main.c:1047:3: warning: conversion frominttounsigned charmay change value [-Wconversion]
   snd_seq_ev_schedule_tick(&ev, SND_SEQ_QUEUE_DIRECT, 1, 0);
   ^~~~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1051:3: warning: conversion frominttounsigned charmay change value [-Wconversion]
   snd_seq_ev_schedule_tick(&ev, SND_SEQ_QUEUE_DIRECT, 1, 0);
   ^~~~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1055:3: warning: conversion frominttounsigned charmay change value [-Wconversion]
   snd_seq_ev_schedule_tick(&ev, SND_SEQ_QUEUE_DIRECT, 1, 0);
   ^~~~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1059:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
     case 0x8: snd_seq_ev_set_noteoff(&ev, channel, byte1, byte2); break;
               ^~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1059:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1059:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1059:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1060:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
     case 0x9: snd_seq_ev_set_noteon(&ev, channel, byte1, byte2); break;
               ^~~~~~~~~~~~~~~~~~~~~
tui_main.c:1060:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1060:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1060:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1061:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
     case 0xb: snd_seq_ev_set_controller(&ev, channel, byte1, byte2); break;
               ^~~~~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1061:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
tui_main.c:1061:15: warning: conversion tounsigned intfromintmay change the sign of the result [-Wsign-conversion]
tui_main.c:1062:15: warning: conversion frominttounsigned charmay change value [-Wconversion]
     case 0xe: snd_seq_ev_set_pitchbend(&ev, channel, (byte2<<8) | byte1); break;
               ^~~~~~~~~~~~~~~~~~~~~~~~
tui_main.c:1062:15: warning: conversion frominttounsigned charmay change value [-Wconversion]

i fired up VCV Rack to do some testing and it worked fine, it's nice to see ORCA listed as orca:orca 128:0 in the ALSA devices. i'd like to have this PR merged after some cleaning (or merged and then cleaned).

Also the README.md would need to be updated as different packages are needed to build with portmidi or alsa, and you have to choose one.

@caropf
Copy link
Author

caropf commented Apr 24, 2020

This mixes tab characters into the source code. It also includes non-English comments and text. It also makes some stealthy edits and changes that it shouldn't. You change the target arch from nehalem to native, ignoring the detailed and important comment above but also leaving the comment in place, making it a contradictory comment. And you don't mention this change anywhere or provide a justification for it.

Sorry, I forgot to revert the target arch back to nehalem before commiting. About the other points you mentioned, I can fix them and the conversion warnings mentioned by @npisanti.

The commit for fixing the linking error in @caropf machine introduces a linking error on mine, as i don't have the tinfow installed and it is not required for building orca-c on my distro ( Debian 10 ).

It seems each distro handles tinfo their own way. Gentoo installs both libtinfo.so and libtinfow.so, with the latter being installed only if unicode is enabled when compiling ncurses, while on debian it seems libtinfo installs libtinfo.so only. Maybe a solution is to check if libtinfow is installed before deciding if linking against it is (possibly) necessary.

@Vixeliz
Copy link

Vixeliz commented Oct 19, 2020

I am attempting to use this as I do not have java on my system and as far as I can tell you can't build portmidi without java. However it does not have any sound what so ever for me.

@cancel
Copy link
Collaborator

cancel commented Oct 21, 2020

Most distros have a version of PortMidi with the Java build configuration system stripped out, I think.

Also PortMidi sends MIDI messages. MIDI is not audio. It doesn't make any sound.

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

Successfully merging this pull request may close these issues.

Linking error on linux
4 participants