Add network support by omersiar #747
Conversation
Add network support
|
Build for testing: |
|
https://github.com/dwhinham/mt32-pi/wiki/Networking%3A-RTP-MIDI-%28AppleMIDI%29 says
In the meantime, the awesome @rsta2 has implemented this. Do you think you could incorporate it?
This means that we could advertise both RTP-MIDI and FTP (if enabled) on the network, making it much simpler to connect (without having to remember the IP address). |
|
Build for testing: |
|
One can already connect to minidexed by its hostname (defaults to "minidexed") if the router supports home network domain. |
|
Yes, but that won't cause it to show up in music or FTP applications on the PC/Mac automagically. So far we are not using the mdns publisher in MiniDexed yet at all, so it would be a matter of porting it over from https://github.com/rsta2/circle/tree/master/test/mdns-publisher. Probably something for a later/separate pull request? |
|
Oh sorry I meant the circle as a submodule. Currently minidexed do not checkout to required point for <circle/net/mdnspublisher.h> |
|
Probably have to adjust https://github.com/probonopd/MiniDexed/blob/main/submod.sh so that the needed branch/commit is checked out? We are using this script since editing git submodules on the GitHub web interface is less than user friendly. |
|
Yeah this ⬆️, but changing this can break things. Let me check if latest circle release does break anything. |
|
Testing on a Windows 11 PC: After the usual it says (Minor nitpick: The Trying to connect via FTP from the FTP client built into Windows, I cannot get a connection using I do see a Now I'll move on to testing the MIDI aspects... |
|
It compiles with Step48 with these warnings: net/ftpworker.cpp: In member function 'bool CFTPWorker::_ZN10CFTPWorker4PortEPKc.part.0(const char*)':
net/ftpworker.cpp:464:9: warning: 'char* strncpy(char*, const char*, size_t)' specified bound 512 equals destination size [-Wstringop-truncation]
464 | strncpy(Buffer, pArgs, sizeof(Buffer));
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/ftpworker.cpp: In member function 'const TDirectoryListEntry* CFTPWorker::BuildDirectoryList(size_t&) const':
net/ftpworker.cpp:363:11: warning: 'char* strncpy(char*, const char*, size_t)' specified bound 6 equals destination size [-Wstringop-truncation]
363 | strncpy(VolumeName, VolumeNames[i], sizeof(VolumeName));
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
minidexed.cpp: In member function 'void CMiniDexed::SetVoiceName(std::string, unsigned int)':
minidexed.cpp:1656:9: warning: 'char* strncpy(char*, const char*, size_t)' specified bound 10 equals destination size [-Wstringop-truncation]
1656 | strncpy(Name, VoiceName.c_str(),10);
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../Synth_Dexed/src/dexed.cpp: In member function 'void Dexed::setName(char*)':
../Synth_Dexed/src/dexed.cpp:1699:10: warning: 'char* strncpy(char*, const char*, size_t)' output may be truncated copying 10 bytes from a string of length 155 [-Wstringop-truncation]
1699 | strncpy(name, (char*)&data[DEXED_VOICE_OFFSET + DEXED_NAME], 10);
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
I wanted to give impression that this is not stuck and move the encoder one step to see actual menu. Dirty hack. |
Yes, found a dirty way Command: RETR wpa_supplicant.conf |
net/ftpworker.cpp: In member function 'bool CFTPWorker::_ZN10CFTPWorker4PortEPKc.part.0(const char*)':
net/ftpworker.cpp:466:9: warning: 'char* strncpy(char*, const char*, size_t)' specified bound 512 equals destination size [-Wstringop-truncation]
466 | strncpy(Buffer, pArgs, sizeof(Buffer));
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/ftpworker.cpp: In member function 'const TDirectoryListEntry* CFTPWorker::BuildDirectoryList(size_t&) const':
net/ftpworker.cpp:365:11: warning: 'char* strncpy(char*, const char*, size_t)' specified bound 6 equals destination size [-Wstringop-truncation]
365 | strncpy(VolumeName, VolumeNames[i], sizeof(VolumeName));
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../Synth_Dexed/src/dexed.cpp: In member function 'void Dexed::setName(char*)':
../Synth_Dexed/src/dexed.cpp:1699:10: warning: 'char* strncpy(char*, const char*, size_t)' output may be truncated copying 10 bytes from a string of length 155 [-Wstringop-truncation]
1699 | strncpy(name, (char*)&data[DEXED_VOICE_OFFSET + DEXED_NAME], 10);
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
Build for testing: |
|
I'm pretty happy with this PR overall now. In the log I see very frequent The advertised service is recognized properly by the host computer. Is this expected @omersiar? |
|
It should be the other way around, if the syslog class expects a string then config should return a string. |
|
Build for testing: |
|
Well, I'm not going to touch syslog anymore since it's working now :) |
This is due if network connection is not reliable, don't you get also the network disconnected messages? If that is not the case then network stack thinks it is connected but packages may not go through. |
|
Thing is, it's playing MIDI coming in over rtpMIDI nicely, and I can upload firmware no problem. No network disconnected messages either. So for me it's purely cosmetic. This is on a Raspberry Pi Zero 2 W. Maybe we could increase some timeouts and/or TTL? |
|
@diyelectromusic please review this if you have a chance. I'd like to get it merged before master deviates too much. Alone the possibility to output logs somewhere else than HDMI proves really useful. |
|
Build for testing: |
As I mentioned in the other PR there is far too much code for me to comment on the implications for the MiniDexed codebase. I'm afraid I'm unable to advise on any of this change other than repeat my general concern - adding a range of scheduled tasks to handle network activity will, by definition, interfere with the audio digital signal processing at some point. I just don't know if it will be significant or not, especially if other Pi versions are factored in. How has the networking scheduling scheme been designed (and why)? How will it fit alongside the main ProcessSound loop and architecture of MiniDexed as it stands? Has anyone looked at the process latency this introduces now? If FTPing files over, what does run-time updating of the SD card do to the rest of the processing threads? How will network timeouts be handled and will they suspend processing in odd places? How is concurrency between menu updates and network updates handled (and how do we ensure there are no race-conditions or consistency problems)? And so on... I've not seen any discussion of how the networking stack has been architected into the existing code. And I don't know enough about the architecture of MT32-Pi to know if a straight port of the same approach works here. I've feel like I've managed to get to grips with the core codebase (pretty much), but there is no way I can try to get inside this code, so if you plan on accepting it, I think you need to think very carefully about who is going to maintain it and debug it (and the rest of the system now having to cope with it of course) if there are issues further down the line. Kevin |
|
Yes, I understand your concerns @diyelectromusic. I view this whole networking code as an opt-in feature that should, if switched off, have no impact. So that whenever an issue comes up, we can compare with networking switched off. I've tested this quite a bit over the last days. And no, I don't advise on uploading files over FTP during a live performance ;-) |
|
Build for testing: |
|
Ok, so it would appear (according to the Scheduler documentation in Circle) that the non-pre-emptive, cooperative multitasking scheduler will only ever work on core 0 in a multi-core system... So in that case, these networking tasks cannot interfere with the audio DSP side of things which all happen on cores 1,2 and 3. Any potential issues will be in things like UI handling and handling of non-networked MIDI, so it would be good to know if the USB and serial MIDI handling are affected by the networking; and then how the UI functions whilst the networking is running. Aside: I think this call to ->Yield() probably shouldn't be there - this is in the non-multicore (so Pi V1) area of code: I'm also not clear why there are so many calls to Yield in the CMiniDexed::Process() - e.g. in every part of the performance file handling. Was this to fix some issues that have been found with the scheduling? I really think this needs some developer documentation adding to the wiki to explain how the CTask and CScheduler facilities of circle are being used to support the networking. I'd also like to see more checks/comments in the networking code to make it clear what is called when and to make sure things will drop out if things aren't properly initialised. There seems to be a few assumptions about order and call sequences to ensure that the networking isn't enabled when not configured. Should any of this sequencing change in the future (I don't know why it would, but things do shuffle around with time) then those assumptions would break. Another Aside: I think printf() was used in mididevice.cpp as LOGNOTE doesn't always work in all contexts - e.g. if called from a serial port interrupt or something like that - I forget exactly... (although I'm not sure those printfs are particularly useful in there anymore anyway...) I'm not sure I quite follow the logic in CMiniDexed::UpdateNetwork() - is that just performing the network startup? If so, why is it called for every scan of Process()? I'm not following the interplay between bNetIsRunning, m_bNetworkInit, and m_bNetworkReady - can we make this a proper state machine if these are states the system passes through - and are these states appearing on separate calls to UpdateNetwork() or are they all expected to happen within the same call? Are those LOGPANIC calls really the most appropriate response to a networking issue? Also, response to various other bits of the networking failing seem inconsistent (there is no checking at all for m_UDPMIDI.Initialize() for example). btw - should we be saying "applemidi" - wouldn't it be better to say rtpmidi... (and we'd not be building someone else's company name into the source of an open project...)? There seem to be some build updates as part of these changes that don't seem specific to this change? I've mentioned before that I'm really not a fan of on-the-fly patching of major source files. If you really want to build the git hash into the firmware, can we put it in its own include file or something so we're not trying to patch userinterface.cpp? Wouldn't need a s// then either - just echo it out to the file. (isn't "." a wildcard anyway? So wouldn't this actually patch every occurrence of the string "Loading" and any following three characters no matter what they are? Which could even include a newline and the start of the next line iirc...) As I say, I've not been able to follow the networking code itself, I've just tried to think through any ramifications for the core MiniDexed functionality... Kevin |
|
If we already have wifi, ftpd, syslog, I think we should also have sshd :P Has anyone tried to use 8x synth_dexed on Linux? I think it could be interesting with PREEMPT_RT. |
|
I don't think synth_dexed has been ported to Linux. Might be a fun experiment for someone, though. If it is done in a way that keeps dependencies minimal (e.g., only ALSA, no jackd) it might run on a very stripped down "embedded Linux" type system containing not much more than the kernel, but allow for easier implementation of things like USB gadget mode as a sound device, MIDI device, and at the same time USB host for MIDI controllers, mass storage, etc. (I think) and networking, including ssh. Let me know if you ever start experiments in that direction. But that'd be a completely new development unrelated to today's MiniDexed. When I started MiniDexed I wanted something small and efficient, and not gigabytes of software with thousands of files. |
|
Build for testing: |
Continuation from #744
This adds wired and wireless network for supported models:
Closes #43
Thanks @omersiar
This allows us to send MIDI e.g., from DAWs, Dexed on the PC, or even sysex for voices from sites like patches.fm
Documentation:
https://github.com/probonopd/MiniDexed/wiki/Networking#ethernet-and-wlan-support