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

Connecting to DHD devices with an (audio)Matrix deliver exception #43

Closed
radio42 opened this issue Apr 7, 2017 · 26 comments
Closed

Connecting to DHD devices with an (audio)Matrix deliver exception #43

radio42 opened this issue Apr 7, 2017 · 26 comments
Assignees
Milestone

Comments

@radio42
Copy link

radio42 commented Apr 7, 2017

Connecting to any DHD device with e.g. an Audio-Matrix throws a ModelException "Inconsistent source or target counts in matrix".
Is this a DHD issue or an issue inside this lib?
The legacy EmberPlusViewer 1.6.2 displays the Matrix without any issue or warning.

matrixerror

@andreashuber-lawo andreashuber-lawo self-assigned this Apr 9, 2017
@andreashuber-lawo andreashuber-lawo added this to the v1.4 milestone Apr 9, 2017
@andreashuber-lawo
Copy link
Contributor

Is this a DHD issue or an issue inside this lib?

Hard to say. Could you please attach a log file? Thanks!

@radio42
Copy link
Author

radio42 commented Apr 9, 2017

Here is one:
20170409_083127_UTC.txt

@radio42
Copy link
Author

radio42 commented Apr 9, 2017

Even when I comment the 'throw exception' in your lib (in MatrixBase`1.cs), I am still unable to get all nodes from DHD devices .. .even in DirectOnly mode using your exact 'TestBug40' code (receiving the childs recursively one by one)...
Here is another log... so I really don't know what to do anymore...

20170409_084927_UTC.txt

@andreashuber-lawo
Copy link
Contributor

Apparently, the DHD provider reports each target and source within its own matrix. That's certainly a very unusual (and inefficient) way of sending a matrix but I'm not sure whether it's illegal or not. I'll get back to you on Tuesday.

@radio42
Copy link
Author

radio42 commented Apr 10, 2017

Note, that the legacy Ember+ Viewer 1.6.2 can actually display and edit all given DHD matrices.
So shouldn't this lib be able to do the same - or is the legacy Ember+ Viewer 1.6.2 also behaving 'illegal'?

@andreashuber-lawo
Copy link
Contributor

andreashuber-lawo commented Apr 10, 2017

Note, that the legacy Ember+ Viewer 1.6.2 can actually display and edit all given DHD matrices.
So shouldn't this lib be able to do the same - or is the legacy Ember+ Viewer 1.6.2 also behaving 'illegal'?

No, it's one thing to accept "incorrectly" formatted Ember+ data. It's an altogether different thing to send out "incorrectly" formatted Ember+ data. The former is perfectly acceptable, the latter is not.

I use quotes because I don't know yet whether what DHD sends out is really incorrect. As I said, it's certainly highly unusual and inefficient.

@radio42
Copy link
Author

radio42 commented Apr 10, 2017

I understand your point, but this is exactly what I tried to explain in my previous issue #40.
If (theoretically) the legacy Ember+ Viewer 1.6.2. accept "incorrectly" formatted Ember+ data - this might providers (like DHD) make believe, that they are sending "correct" data.
Thus you could argue, that all ember implementations should behave the same and accept "incorrectly" formatted Ember+ data (which I guess is not what is wanted).
The only way to stop this is to not accept "incorrectly" formatted Ember+ data at all.

But as you indicated - let's first wait, if Lawo/you believe if its "illegal" data or not.

Depending on this outcome, my expectation would be:

a) if its a DHD bug: Lawo should contact DHD and explain, why its illegal and suggest a fix.
In addition I would suggest to urgently change the behavior of the legacy Ember+ Viewer to also reject this "incorrect" data (so that other providers might not fall into the same 'trap').

b) if its a bug of this lib, it simply needs to accept this "correct" data (even its unusual).

@DHDaudio-support
Copy link

I upload two Glow logs for clarification.
I used the same small device with a little 4*4 matrix.
The first log was generated using the EmberPlusView 1.6.2. The second log was generated using the ProppFrexx Software (which shows the "Inconsistent source or target counts in matrix" message).

20170411_075538_EmberPlusView_SmallDevice.xml.txt
20170411_075733_ProppFrexx_SmallDevice.xml.txt

@andreashuber-lawo
Copy link
Contributor

andreashuber-lawo commented Apr 11, 2017

Thus you could argue, that all ember implementations should behave the same and accept "incorrectly" formatted Ember+ data (which I guess is not what is wanted).
The only way to stop this is to not accept "incorrectly" formatted Ember+ data at all.

While I agree that we could tighten the specification somewhat and provide tools to verify the validity of messages to a certain degree, I think it is close to impossible to make all implementations behave the same. There will always be corner cases where different implementations of the protocol will behave differently.

a) if its a DHD bug: Lawo should contact DHD and explain, why its illegal and suggest a fix.

Other standards bodies (W3C, IEEE, etc.) usually do not attempt to contact vendors who implement its standards incorrectly either. While I'm in no position to take any decision pro or contra, I doubt that we'll generally do what other standards bodies don't. There might be exceptions of course.

In addition I would suggest to urgently change the behavior of the legacy Ember+ Viewer to also reject this "incorrect" data (so that other providers might not fall into the same 'trap').

As far as I understand, the Ember+ Viewer was never intended to work as a validation tool (it's far less work to implement a simple viewer than implementing a validator). While I agree that it would be nice to have validator, the reality is that we currently don't.

After some internal discussion, it appears that we're currently leaning in the direction that it cannot be permissible to report sources and targets the way the DHD provider does (for connections it is permissible). However, the specification does not currently spell this out, so we have some work to do there.

@radio42
Copy link
Author

radio42 commented Apr 11, 2017

Thanks.
But what can I now do to work with DHD devices? Should I wait for a fix in this lib?
Or what do you mean by 'some work to do'?

@andreashuber-lawo
Copy link
Contributor

Or what do you mean by 'some work to do'?

We need to tighten the specification in this area.

@radio42
Copy link
Author

radio42 commented Apr 11, 2017

So you are saying: Unless DHD changes its implementation we cannot use THIS lib to work with DHD !?

@andreashuber-lawo
Copy link
Contributor

andreashuber-lawo commented Apr 11, 2017

So you are saying: Unless DHD changes its implementation we cannot use THIS lib to work with DHD !?

Yes, unless compelling arguments are brought forth why it would make sense to allow this way of defining a matrix.

@radio42
Copy link
Author

radio42 commented Apr 11, 2017

Who should bring up those arguments?

I mean: I am a customer and simply what to leverage Ember+ in my product - like the users of my product and DHD products just want to make it working. And now I feel like I am sitting in between Lawo and DHD - standing in the rain!
Guys, this is something you should sort out internally between DHD and Lawo!

I only understand up to here: DHD is not using Ember+ as it should (it is using "incorrectly" formatted Ember+ data). Or in short: DHD is NOT Ember+ compliant !

@andreashuber-lawo
Copy link
Contributor

Guys, this is something you should sort out internally between DHD and Lawo!

I guess I have to repeat what I said earlier: In the case of Ember+ Lawo acts as a standards body. That is, we maintain and publish the specification. Just like other standards bodies (like IEEE, W3C, etc.) I think it cannot be our obligation to validate, verify or otherwise test the products of vendors who happen to implement the specification. Moreover, I think it can neither be our obligation to contact vendors even if we learn of bugs in their implementations. To my knowledge, other standards committees aren't doing that on a regular basis either. Their reasons, I suspect, are the same as ours: We simply don't have the capacity to do that.

That being said, we are happy to help whomever contacts us with bugs, concerns and questions (to the best of our ability).

I only understand up to here: DHD is not using Ember+ as it should (it is using "incorrectly" formatted Ember+ data). Or in short: DHD is NOT Ember+ compliant !

Again, as I tried to explain above, reality is a bit more complex. According to the current specification, the DHD provider is compliant. However, its unusual way of publishing a matrix has uncovered a bug in our specification. Once we fix said bug, then the DHD provider is no longer compliant.

@radio42
Copy link
Author

radio42 commented Apr 12, 2017

When the DHD provider is currently 'compliant' - what options do I have to still read it's data with THIS lib?
Whatever I tried it is failing...(I even tried to read all elements with the DirectOnly option and try to skip any IMatrix elements, but this is unfortunately already too late).

So is the last option left for me to NOT use this lib anymore?

@DHDaudio-support
Copy link

DHDaudio-support commented Apr 12, 2017

Hi Andreas,

Can you please point out where the DHD matrix uses "incorrectly" formatted Ember+ data"?
But it seems to be more an

unusual way of publishing a matrix

Maybe in comparison with the following GlowProxy log that I've recorded from the TinyEmberPlusRouter-1.6.2) 1:N matrix. - which I'd consider as reference implementation.
Of course the matrix sizes are different (DHD log from above 4x4 and the TinyEmberPlusRouter 200x200).

20170410_160220_EmberPlusView_TinyEmberPlusRouter1toN.xml.txt

As far as I see the structural matrix definition itself must be correct, as our matrix implementation was verified against several software like VSM Studio, Axon Cerebrum and the Ember+Viewer.

Will you post here as soon as it is clear how the current specification will be revised in order to fix the "bug" in the specification?

@radio42
Copy link
Author

radio42 commented Apr 12, 2017

And maybe a 'tiny', 'global' option in your lib to 'ignore and skip' all 'invalid' matrix elements would also help to overcome the issue - up until:

  • you come out with a new specification
  • and all other vendors have adopted this in their products

@andreashuber-lawo
Copy link
Contributor

And maybe a 'tiny', 'global' option in your lib to 'ignore and skip' all 'invalid' matrix elements would also help to overcome the issue - up until

You already have that option, you can use the latest published release of this library (which doesn't yet support matrices).

@radio42
Copy link
Author

radio42 commented Apr 12, 2017

But this doesn't contain your other fixes added later?!
AND...I am developing a standard product, which should work with ANY provider!
I want to only use one standard lib and not 'lib-version1' when connecting to DHD and using another 'lib-version2' when dealing with other providers.
That's why I would need this as an option in the product - and as such in your lib.
With such an 'option' I could at least support all providers and tell my end-customers to enable this option in case a providers uses 'incorrect' matrix data.

@andreashuber-lawo
Copy link
Contributor

andreashuber-lawo commented Apr 12, 2017

Will you post here as soon as it is clear how the current specification will be revised in order to fix the "bug" in the specification?

Looking at 20170409_084927_UTC.txt, after the matrix details are requested in consumer message no. 5, the DHD provider currently responds with a great number of messages (no. 8-246 in the log), each containing one target, source or connection.

It looks like we're going to change the specification such that it becomes invalid to report the sources and targets of a matrix in multiple messages. That is, the provider must respond with a message that at least contains all targets and all sources of the matrix. Since connections can change anyway over the lifetime of an Ember+ session, no such requirement exists for connections, but of course it would be more efficient to also report them in the same message.

Mind you, this is all preliminary, so I cannot guarantee that we're indeed going to change the specification, as doing so requires the agreement of many stakeholders. However, I would suggest you change the behavior of the provider in any case, as the current way of publishing a matrix is highly inefficient.

@andreashuber-lawo
Copy link
Contributor

So is the last option left for me to NOT use this lib anymore?

Apparently, you require a level of control that this library is not designed to give you. I would therefore suggest you use the .NET library that comes with the official Ember+ release.

@radio42
Copy link
Author

radio42 commented Apr 12, 2017

So is this lib not official?
You might remember, that I asked this initially and the answer was a very clear 'YES' (afair).

However, I guess this discussion has come to an end and is not worth any further arguments.
We found two important bugs: one on the DHD side and one in the Ember+ specs itself!
And I guess I have to live with these results... even if they are not really satisfactory.
I hope, that DHD can provide respective fixes quite soon.
And I will carefully follow the announced change of the "Ember+ specs" with regards to matrix elements.

@radio42
Copy link
Author

radio42 commented Apr 12, 2017

You might close this issue.
It took me 2 hours to study your source code, and then 5 minutes to add a global option to ignore/skip all matrix elements if needed - only 4 little source code changes where necessary.
As such: it was not so hard as you indicated and the 'level of control' I needed was totally easy to implement - that's why I cannot understand your reaction 2 posts above.

This way I can even support all DHD devices, just matrices are not shown, but I can live with this, until you come up with new specs and DHD with a new firmware.
If anyone is interested in my code changes, just give me a ping.

@andreashuber-lawo
Copy link
Contributor

As such: it was not so hard as you indicated and the 'level of control' I needed was totally easy to implement - that's why I cannot understand your reaction 2 posts above.

Not sure how I supposedly indicated that it was hard to implement. I wrote that the library is not designed to give you that level of control. I stand by that statement. The main goal of the library is ease of use, which almost always comes at the expense of versatility. The library already offers a bunch of options that make things more difficult than I would like. Consequently, adding yet another option for a hopefully short-lived problem is not something I do on a whim.

Never mind the fact that we provide this free of charge with a very permissive license, so you are free to make whatever changes you like and even republish it.

@andreashuber-lawo
Copy link
Contributor

So is this lib not official?
You might remember, that I asked this initially and the answer was a very clear 'YES' (afair).

No, I don't but I'm sure you can point me to the place where I've said that. The only thing I do remember is that you expressed your hope that the library will be officially supported, I quote:

I hope it will officially be supported also in the future, as I plan to use it within my radio automation software (ProppFrexx ONAIR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants