API extension to support user-selectable network ports

[fduncanh]

Since Waester's pull request #196 hasn't been worked on to implement the command line option that FD- and pallas requested, I have done it in this pull request.

(the original pull request is reworked, as the ports can now be changed with the command line option -p[n].)

-p uses the hard coded legacy ports found by Waester. -pn uses both tcp and udp ports n, n+1, n+2 in range 1024- 65535

This is now implemented through an addition to the API of the airplay RAOP library to allow port variables to be selected

The help option and documentation are also updated. tested by mirroring an iPad running iPadOS 14.6.

[fduncanh]

updated to expand the -sp option so -sp activates Waester's hard-coded legacy port choices, and -sp[n] (e.g. -sp34567) changes the static ports to tcp AND udp in range n-(n+2) (e.g. 34567-34569 , 3 udp and 3 tcp ports). where ports are in the "ephemeral port" range 32768-65535,

[FD-]

Thanks for your contribution! Here are a few remarks I have regarding your changes:

• I think a cleaner solution for setting ports would be passing them (or the first in a sequential series) as arguments to the init functions (0 to pick randomly, but that comes naturally anyway). That way, the different components would not have to deal with the port decision policy at all. What do you think?
• Please fix your indentation (you used tabs in a few places) and put a space char between if and the condition.

[fduncanh]

Hi FD, I redid the structure in a clearer way.

-p shows ports to be used and stops default ports are now Waester's set of six (3 tcp, 3 udp) "legacy" hard coded ports -pn changes ports to tcp n n+1 n+2 udp n n+1 n+2 in range 32768-65535 -pn -p shows the ports that -pn selects.

special case -p0 restores earlier dynamical (random) port assignment of the current rpiplay I go back and check for tabs etc

[fduncanh]

I thing the tabs are all gone, "if"'s followed by space etc

[fduncanh]

code is cosmetically "squeeky clean" now, and tested. ready to merge.

[FD-]

Thanks for the rework! Still, I'd prefer a solution where ports are passed as arguments to the init functions (0 to pick randomly, but that comes naturally anyway), so that we don't need a single static variable, which defeats the purpose of the class-like struct.

[fduncanh]

FD-, I'm not sure I understand what you want, but I will look into it. I'tt create a struct with pointers to the port values.

[FD-]

Sorry, I should have explained in detail. What I'm suggesting is placing the port variables inside the individual structs (raopntp, raop*) instead of having them as separate globals. The library in theory supports creating multiple instances of these components, which is no longer possible if the port is a single global variable (per struct type).

In object-oriented terminology, I want you to make the ports instance fields instead of static class fields.

The field inside each struct would then be filled from a parameter to the respective init function.

Hope this makes sense! Let me know if you have further questions!

fduncanh @.***> schrieb am Mi., 16. Juni 2021, 08:50:

FD-, I'm not sure I understand what you want, but I will look into it. I'tt create a struct with pointers to the port values.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/FD-/RPiPlay/pull/249#issuecomment-862097622, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZKX5VDU67CSJEVUD27GGDTTBCT5ANCNFSM46TPUGKA .

[fduncanh]

"The field inside each struct would then be filled from a parameter to the respective init function."

But this seems to be exactly what the code was always doing?

I think you misunderstood what my code was doing, because of my ugly hack of using a global "extern" variable in lib/raop_ntp.c etc (it was only used for the initialization process, it wasn't in any way storing the port data in a global variable).

I removed it, and now use a function call to a function initialize_port in rpiplay.cpp for initializing the init functions.

The created port address is indeed stored in structs like raop_ntp_s

Please take a new look an my code. and let me know.

I replaced include "raop_ntp.h" with "include raop.h" in raop_ntp.c (raop.h includes raop_ntp.h) in put the initialization function prototype there

[FD-]

I now had a closer look through the existing codebase (it's been a while since I worked on the project). The details I describe below might differ from what I wrote above, but the idea remains the same.

My main point is: The library currently supports multiple connections at once (even if the renderers don't support that), each with its own instance of raop_ntp, raop_rtp and raop_rtp_mirror. These are in the raop_conn_s structs. I'd like to retain that architecture, which means that each individual connection needs its own set of ports for raop_ntp, raop_rtp and raop_rtp_mirror.

AFAICT, your current design doesn't cover that.

Additionally, I don't like how you currently have your declarations and implementations all over the place, breaking the conceptual separation between the library and the program. Generally, please keep the declaration in the header corresponding to the file that implements the function, otherwise we just get a huge mess over time.

[fduncanh]

I believe that the change I proposed maintains the separation between the library and the rpiplay.cpp program.

From the library's viewpoint, in my implementation it just knows about a "user-supplied" function with a prototype

void initialize_port(unsigned short *port, unsigned short port_id)

(now) defined in a new include raop_port.h , where \"port_id\" takes one of 5 values also defined in raop_port.h, which is now included in raop_ntp.h and in raop.h Here the function initialize_port is supplied in rpiplay.cpp.

The only change to the current header files is now the inclusion of raop_port.h in raop.h and raop_ntp.h. (I reversed the previous change in raop_ntp.c, which now again includes raop_ntp.h and not raop.h)

If you wish, the name of the \"user supplied\" function could be changed to raop_port()

If the library is used with multiple instances, the user supplied initialize_port(..) function can for example be modified to update the the non-zero value of the static variable in the calling program that it takes the values from by 3 on every call.

Since rpiplay only calls the initialization one time, I did not include this in my implementation of initialize_port in rpiplay.cpp.

For example such an implementation could contain :

case  UDP_DATA_PORT
   *port = AUDIO_DATA_UDP_PORT;
    /* if it is not 0, add code here for updating the static variable AUDIO_DATA_UDP_PORT to a new value */
     break;

If you don't like capitalization of the static port variables in rpiplay.cpp like AUDIO_DATA_UDP_PORT I can change this.

[fduncanh]

Looking at this some more, maybe it is best to add the proposed set of five ports calculated from user input to the main program (rpiplay) as a new input argument to start_server e.g. &network_ports, an array of 5 port numbers.

[fduncanh]

I'm still not done.... I have placed an array of the initial port values as an argument of start_server(), but my current code still makes them static because of an include.

I now see what you mean about getting them into the raop_t struct. to get the values down to the init_socket functions.

still working on it.

[fduncanh]

quoting FD-:

_My main point is: The library currently supports multiple connections at once (even if the renderers don't support that), each with its own instance of raop_ntp, raop_rtp and raop_rtp_mirror. These are in the raop_conn_s structs. I'd like to retain that architecture, which means that each individual connection needs its own set of ports for raop_ntp, raop_rtp and raop_rtpmirror.

FD- I finished doing exactly this (If I understood you) , this weekend. As you suggested, I managed to understand how to properly inject the ports requested into the init structures. I had to work out how to store the port requests in the structure raop_t so they could be injected into raop_ntp_t raop_rtp_t and raop_rpt_mirror_t when the handler calls the init functions..

The API now contains functions for writing the requested port-values into raop_t immediately after it has been created by raop_init (but before raop_start is called ) in start_server. The API now contains set_timing_port(raop,port) etc in addition to set_port,

The essential feature is the ability to seed a given instance of raop_t with a full list of ports to use, just after it has been created.

The changes are tested and work.

[fduncanh]

This got automatically closed when I reset my repo for a fresh start. The code written as FD- requested is in a new pull request.