jack2 icon indicating copy to clipboard operation
jack2 copied to clipboard

Avtp pr

Open chris-kuhr opened this issue 6 years ago • 47 comments

chris-kuhr avatar Apr 08 '19 11:04 chris-kuhr

@chris-kuhr WOW - this looks so much better! There are some (minor) build issues for OSX we have to sort out, here

https://travis-ci.org/jackaudio/jack2/jobs/517218532#L3544

In file included from ../linux/avb/mrp_client_send_msg.c:19:
../linux/avb/mrp_client_send_msg.h:35:10: fatal error: 'malloc.h' file not found
#include <malloc.h>
         ^
1 error generated.

One around C99 compat:

/avb/avb.c.7.o']
../linux/avb/avb.c: In function ‘check_stream_id’:
../linux/avb/avb.c:112:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for(int i = 0; i < 8 ; (*buf_offset)+=2, i++)  {
     ^
../linux/avb/avb.c:112:5: note: use option -std=c99 or -std=gnu99 to compile your code
../linux/avb/avb.c:119:13: error: redefinition of ‘i’
     for(int i = 0; i < 8 ; i++)  {
             ^
../linux/avb/avb.c:112:13: note: previous definition of ‘i’ was here
     for(int i = 0; i < 8 ; (*buf_offset)+=2, i++)  {
             ^
../linux/avb/avb.c:119:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for(int i = 0; i < 8 ; i++)  {
     ^
../linux/avb/avb.c: In function ‘check_listener_dst_mac’:
../linux/avb/avb.c:144:5: error: ‘for’ loop initial declarations are only allowed in C99 mode
     for(int i = 0; i < 6 ; (*buf_offset)+=2, i++)  {
     ^

For the latter it's enough to declare 'i' outside of the loop IIUC.

I've scanned over the files and something that popped out was extensive spacing like here:

https://github.com/jackaudio/jack2/blob/572758dfc80c3a490236bd125d4eed39c5f6457e/wscript#L540

These are minor things. From my perspective this looks otherwise good so far, thank you for taking it further! I'll take a closer look and let's give it some time for others to check too.

7890 avatar Apr 08 '19 19:04 7890

@chris-kuhr there are many indentation / white space / consistency issues.

I've "fixed" formatting for all avb/*.h files, see attachment. You can apply this to your PR where you see fit. For the implementation files: please review them mechanically for formatting issues. The minimum requirement is that within one (new) file, indentation should be either space or tab and never both. It would be wishful if the same decision is made for all files in avb/.

If you don't have the motivation to make the last steps, let us know - somebody can do code formatting but it will take some time. The goal is that the code gets into the repository without any known issues (technical and form) for the initial addition. I was speaking just of form.

Thanks again! avtp-pr_headers.diff.gz

7890 avatar Apr 09 '19 04:04 7890

Tomorrow, I have a long trip by train ahead of me , I will look into it. What ist more preferable space or tab?

I have no way to test OSX (and no interest). Thus, it should be excluded from the build process. Why are contents of the linux folder compiled under OSX at all?

I added the empty line to wscript to find it again. I'll remove them.

Best, Christoph

chris-kuhr avatar Apr 09 '19 07:04 chris-kuhr

It's really up to you :)) - there are no code guidelines (...). It should be consistent in some way for new files, for existing files the strategy is to follow the current style.

For the OSX build issues: it seems a small step to make it compile. For one problem, the solution is very simple and for the malloc.h a quick google search shows that it can be solved with an ifdef and using stdlib.h on OSX. The travis builds do the work for us in telling if it compiles (no need to have an Apple computer nearby). It's a first indication that the backend can possibly work on OSX which is more similar to Linux than to Windows.

Related to .h files: It's recommended to include only the headers needed for a single unit. Headers are included very broadly now. Including many files in a header file which itself isn't dependent on the includes should be prevented. Instead the includes needed by implementation can go to implementing .c files. I've found this an interesting read https://www.acodersjourney.com/top-10-c-header-file-mistakes-and-how-to-fix-them/ for inspiration. Not sure if this should be considered now or later.

And then have a nice train journey!

7890 avatar Apr 09 '19 10:04 7890

Ok, a successful compilation and execution are different things. I have no way of testing an OSX setup. That's what I wanted to say...

The header files show the experimental character... ;-)

chris-kuhr avatar Apr 09 '19 11:04 chris-kuhr

@chris-kuhr I've checked out your branch and indeed there is no single tab left in any of the avt/* .h or .c files, very well. I'm nitpicking on more formal aspects:

-Consistency of header guards -Consistency of spacing

A simple thing to do is to open all files in sequence *.h and then see if every file's first 40-60 lines are equally layed out, this includes the same license header (which seems to be the case), a common way to name the header guards (eg. underscorenameunderscore everywhere), a common way to close the guards inclusive comments.

When looking at all files at a glance https://github.com/jackaudio/jack2/pull/452/files the goal is to not be able to spot anything "odd", currently it's still easy to spot extensive line spacing. In the end all commits can be squashed to one (that will be falktx' magic) so it's not an issue to have whitespace commits in the PR, IIUC it will go to develop branch in one (consolidated) commit as a single addition.

Still on the list is to prevent for (int i... egrep "for\(int" * shows a few instances, easy to fix. Interestingly enough, doing that grep on all of jack's code shows many instances of this sort. It's unclear why those won't create errors in the various tarvis builds (???).

For the header including other headers, it's much better. Some more potential, I take a random header mrp_client_send_msg.h. I don't see why it would include malloc.h.

@ahellquist: IIUC you have a setup involving AVB. It would be great if you could give it a spin. git clone -b avtp-pr https://github.com/chris-kuhr/jack2

We will also need an updated manpage, I can take care of this. I think we're getting close!

7890 avatar Apr 11 '19 00:04 7890

Den tors 11 apr. 2019 kl 02:59 skrev 7890 [email protected]:

@ahellquist https://github.com/ahellquist: IIUC you have a setup involving AVB. It would be great if you could give it a spin. git clone -b avtp-pr https://github.com/chris-kuhr/jack2

I have two Ultralite AVs, an AVB switch and a few boxes of i210s waiting to be tested so it is not exactly a running setup utilizing AVB. My standard setup is One AVB card connected directly to my gig laptop via usb My plan is to test this but it will probably take some time to set this up on my side and I am a bit short of time.

We will also need an updated manpage, I can take care of this. I think we're getting close!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-481925408, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvLy-Z44FAPcqT15C-l7M9SsP_1jEDks5vfojXgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 06:04 ahellquist

I did try to build jack2 from the chris-kuhr repo but my local clone does not have any files under linux/avb/OpenAvnu OpenAvnu seems to be outside the repo

git status gives

On branch avtp-pr Your branch is up-to-date with 'origin/avtp-pr'. nothing to commit, working tree clean

My guess is I have to clone with other arguments to get the full tree needed.

/Anders

Den tors 11 apr. 2019 kl 02:59 skrev 7890 [email protected]:

@chris-kuhr https://github.com/chris-kuhr I've checked out your branch and indeed there is no single tab left in any of the avt/* .h or .c files, very well. I'm nitpicking on more formal aspects:

-Consistency of header guards -Consistency of spacing

A simple thing to do is to open all files in sequence *.h and then see if every file's first 40-60 lines are equally layed out, this includes the same license header (which seems to be the case), a common way to name the header guards (eg. XXX everywhere), a common way to close the guards inclusive comments.

Still on the list is to prevent for (int i... egrep "for(int" * shows a few instances, easy to fix. Interestingly enough, doing that grep on all of jack's code shows many instances of this sort. It's unclear why those won't create errors in the various tarvis builds (???).

For the header including other headers, it's much better. Some more potential, I take a random header mrp_client_send_msg.h. I don't see why it would include malloc.h.

@ahellquist https://github.com/ahellquist: IIUC you have a setup involving AVB. It would be great if you could give it a spin. git clone -b avtp-pr https://github.com/chris-kuhr/jack2

We will also need an updated manpage, I can take care of this. I think we're getting close!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-481925408, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvLy-Z44FAPcqT15C-l7M9SsP_1jEDks5vfojXgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 17:04 ahellquist

it's a submodule. you need to get it with 'git submodule update --init'.

chris-kuhr avatar Apr 11 '19 17:04 chris-kuhr

Built successfully without dbus

Started jackd and tried to run a script called start_jackd_avtp_backend.sh

Oput: jackdmp 1.9.12 Copyright 2001-2005 Paul Davis and others. Copyright 2004-2016 Grame. Copyright 2016-2018 Filipe Coelho. jackdmp comes with ABSOLUTELY NO WARRANTY This is free software, and you are welcome to redistribute it under certain conditions; see the file COPYING for details no message buffer overruns no message buffer overruns no message buffer overruns Unknown driver "avb"

How can I verify I am running the fresh jackd ?

(Still no i210 card in the computer)

/Anders

Den tors 11 apr. 2019 kl 19:57 skrev Christoph Kuhr < [email protected]>:

it's a submodule. you need to get it with 'git submodule update --init'.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-482226945, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvLwtOk-zXsdcxtEkJ3od-ba525ekbks5vf3dvgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 18:04 ahellquist

you need i210 for this to work.

chris-kuhr avatar Apr 11 '19 18:04 chris-kuhr

you also need a media clock talker on the network, the dest mac address and stream id are mandatory parameters. the name of the i210 device as well.

chris-kuhr avatar Apr 11 '19 18:04 chris-kuhr

also, ptp and mrp daemons are required...

chris-kuhr avatar Apr 11 '19 18:04 chris-kuhr

For reference: https://lac.linuxaudio.org/2019/doc/kuhr.pdf @chris-kuhr do you know if there is the video of your presentation archived somewhere? I missed it and it would be interesting to see.

7890 avatar Apr 11 '19 18:04 7890

My plan was to install the i210 card and hook it up to my AVB switch and one or two Motu AVB interfaces.. Do I need more stuff than the jack2 and the equipment ?

Den tors 11 apr. 2019 kl 20:27 skrev Christoph Kuhr < [email protected]>:

also, ptp and mrp daemons are required...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-482242716, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvL9nUgvhriUPVfdaE7bdk8P_lWoW6ks5vf36DgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 18:04 ahellquist

It's good to test-drive this. These things are important information for anybody else to try.

7890 avatar Apr 11 '19 18:04 7890

@collaborator I don't know.

@ahellquist you need a running OpenAvno installation and a stream you can synchronize to. The talker has to autoconnect the listener, since AVDECC is not implemented.

chris-kuhr avatar Apr 11 '19 18:04 chris-kuhr

CCRMA will do a field test in the near future...

chris-kuhr avatar Apr 11 '19 18:04 chris-kuhr

Ok, I guess I am not fit for the task yet then. Will take a closer look at what I can do to help. I am very glad things are moving in the right direction thoug.

/Anders

Den tors 11 apr. 2019 kl 20:33 skrev Christoph Kuhr < [email protected]>:

@Collaborator https://github.com/Collaborator I don't know.

@ahellquist https://github.com/ahellquist you need a running OpenAvno installation and a stream you can synchronize to. The talker has to autoconnect the listener, since AVDECC is not implemented.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-482244629, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvL8p0onDsSAqo6U5M1U1NyzPt36ZLks5vf3_cgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 18:04 ahellquist

@ahellquist thank you for the first test. I take from this that AVB is somewhat tricky and that a few other units must be in place to successfully use this new backend. It will be useful to add a short documentation on how to use it and a list of prerequisites.

7890 avatar Apr 11 '19 18:04 7890

I think AVB soon will be awesome on Linux and I hope Milan will take off soon too. I thought avdecc was part of this test and had not seen the reference document. My bad

Keep up the good work

On Thu, Apr 11, 2019, 20:43 7890 <[email protected] wrote:

@ahellquist https://github.com/ahellquist thank you for the first test. I take from this that AVB is somewhat tricky and that a few other units must be in place to successfully use this new backend. It will be useful to add a short documentation on how to use it and a list of prerequisites.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/pull/452#issuecomment-482248230, or mute the thread https://github.com/notifications/unsubscribe-auth/AIxvL9FoiKg33g-rmkVStmAUFm2Fvidvks5vf4JFgaJpZM4ch-5Z .

ahellquist avatar Apr 11 '19 18:04 ahellquist

I'm tempted to try this out. @chris-kuhr what are the minimal hardware requirements? From what I read it's -a compatible NIC such as the i210 -a media clock talker on the network (I guess this is a hardware device or can it be a software service?) -daemons running on the hosts Is it for instance a valid setup to have two hosts equipped with i210 running jackd with avb backend, with a direct network link, without any AVB switches or AVB audio interfaces? Could a host with two i210 become an AVB switch? I'm a total AVB noob but I think it will be awesome (like ahellquist). In the meantime I'll read the LAC paper thoroughly.

7890 avatar Apr 11 '19 19:04 7890

this is no complete avb backend!

it can only synchronize multiple jackd processes, running on different avb ready machines, to the same media clock on the avb network. this backend makes no sence in a back to back scenario.

chris-kuhr avatar Apr 11 '19 19:04 chris-kuhr

until now, OpenAvnu can only be used with a single i210 card in one computer.

think of the backend as the avb way of a BNC connector jack for the wordclock. you sychronize to a house clock.

chris-kuhr avatar Apr 11 '19 19:04 chris-kuhr

and you can be sure that your jack audio clients are in sync with the house clock.

chris-kuhr avatar Apr 11 '19 19:04 chris-kuhr

if you want audio i/o, this can be done with some OpenAvnu example jack clients.

chris-kuhr avatar Apr 11 '19 19:04 chris-kuhr

Thanks, that helped. It's like a word clock coming in via network interface ~ Jack client running in AVB enabled jack instances distributed on the network could then for instance do something at almost the same time.

7890 avatar Apr 11 '19 20:04 7890

and I am not shure whether a complete avb backend for jack is desirable. I think a jack client based solution with media clock backend makes much more sense. think of the AVDECC protocol. I think this would be too much for a backend...

you would use the alsa API in the jack avb client for i/o and do some offset calculations from the ptp time to have the proper timing.

this way you could have multiple talkers and listener with audio i/o on a single system. which would not be possible with an avb jack backend...

chris-kuhr avatar Apr 11 '19 20:04 chris-kuhr

I am working on some jack clients, that could bethe basis for that... but there is still a lot of work to do...

chris-kuhr avatar Apr 11 '19 20:04 chris-kuhr

Naively asked, could jack be a media clock talker? Let's say it would provide the clock from the attached audio hardware interface to the network.

To have multiple jack instances synchronized very accurately via network (replacing BNC wordclock) in a robust way opens interesting possibilities. I wondered how AVB handles edge cases, how it translates to jack's timing behavior. If network cable is plugged, network is "overloaded" or just any abnormal operation, how it will recover etc.

7890 avatar Apr 11 '19 20:04 7890