From: Jonathan W. <jw...@ju...> - 2013-10-03 00:44:06
|
Hi Phil On Wed, Oct 02, 2013 at 09:54:33PM +0200, Philippe Carriere wrote: > I found a little bit of time to test. Thanks. > > To this end, could you give the attached patch a go and tell me what > > happens? It should apply to r2415. It's compile-tested only. I think it > > should do the trick, but only if my understanding of the onSamplerateChange > > stuff within ffado-mixer is correct. > > I applied the patch and compiled (over r2147): running ffado-dbus-server > and ffado-mixer leads to the following erors: > - ffado-dbus-server experiences a segmentation fault Bother. That suggests that the eap->update() errored out for some reason in response to the call within getSamplingFrequency(). There's no obvious reason why this shouldn't work so I suspect a bug within eap->update(). Whether it's worth enabling core dumps and firing up gdb to work through this remains to be seen. > - ffado-mixer output was: > : > 21:07:49 panelmanager ERROR error in pollPanels > 21:08:14 panelmanager ERROR The communication with ffado-dbus-server was lost. This would be expected in the circumstances. It's collateral damage caused by the crash of ffado-dbus-server. The real cause is the segfault within ffado-dbus-server; to dig further into that you'll need to enable core dumps and use gdb to inspect the resulting core file to determine where things fell apart. In the first instance it might be instructive to at least obtain a stack trace from ffado-dbus-server. While this approach may not be the best for the situation at hand, the crash may point to a real bug which should be fixed. > where the first three successful mixer update were obtained before jack > start. > As jack starts, and changes the samplerate, the pollPanel error appears. This tells us that the basic functionality is working; it's just that something crashes within ffado-dbus-server in response to the update request from within getSamplingFrequency(). > > This approach does not require the addition of a new control, nor does it > > rely on a dbus client to do the right thing in order to work. > > > > Obviously you see a problem somewhere with this approach and I'd be > > interested to hear what it is. > > My main objection is that getSamplingFrequency() is only intended to be > the way to provide the information of the present samplerate of the > device. While one can expect that, setting the samplerate at a > prescribed value, all is done so as to recover the new configuration of > the device (if it changes with the samplerate), it is really strange > that some implicit modification is also done when just asking for the > present value of the samplerate. That is true and this was in the back of my mind when I was doing the patch up. On the other hand, I don't like the idea that ffado-dbus-server needs to rely on an explicit update signal from a client before it will update its interface. It requires that every client knows that it needs to do this for DICE devices. For now this is academic since there is only one client - ffado-mixer - but this could of course change in future. There probably isn't a "good" solution to this problem. > This will be really disappointing for any future developers (I hope there > will be some) and probably for ourselves when we will forgot somewhat why > we did like this. That's why things like this would be commented within the code. > This is also too much related to the existence of the specific ffado-dbus > interface and I am not sure the approach is able to be robust in any case. True, but the same could be said for the alternative which relies on the client delivering an update request. Any client which doesn't know that this is needed will essentially be broken. > Introducing a new control in CrossbarRouter is not so complex and is not > so much work (it is whatever yet done in my previous patches). Agreed. It's still not a completely robust solution though. > > I haven't analysed it carefully, but I expect the update call within jackd > > has no practical impact. > > No, DICE EAP devices configuration is strongly dependent on the > samplerate ... Fair enough. > What could be a more general approach would be that libffado provides a > kind of isDeviceUpdateOnSampleRateRequired() and a generic function to > do it if required: however, this would be a lot of additional work to do > and control to introduce, with some impact on many devices not involved > in such a behaviour. I would certainly have a preference for a more general approach. It makes this requirement a formal part of the interface specification which means it will be less surprising to other ffado-dbus-server clients which might come along in future. Having said that, I don't think it would be too much extra work to implement this; it should be possible to do this in such a way that the functionality is ignored for devices which don't require it. For example, a virtual boolean isDeviceUpdateOnSampleRateRequired() method (which needs a shorter name) could be added to FFADODevice and by default return "false". This immediately makes this available to all devices without further work being done. DICE drivers would override this method to return "true". The value of this parameter could be made available as an additional generic mixer dbus control in the same way that sample rate and streaming state is already done (see ffadodevice.cpp, FFADODevice::FFADODevice() - it could be just a normal discrete control). This makes the call available to dbus mixer clients no matter what interface is in place, and again requires only a couple of lines of code. The above is just an illustration that the addition of generic controls would not necessarily be a lot of additional work. As I write this, I wonder whether the "isDeviceUpdateOnSampleRateRequired" is really needed. Instead we could have a virtual onSampleRateChange() method defined within FFADODevice which by default does nothing and returns 0 (say). DICE could override this to do whatever it needs to do and return 1 to indicate that changes were done. It could be exposed with a generic dbus control as explained above. On the ffado-mixer side, the generic layer could take care of handling this in much the same way that the DICE-specific patch used to. If a sample rate change is detected then the generic dbus control would be read so the server reconfigures itself if needed. The value read would indicate if a change was made and if so the mixer could be rebuilt. All this would be taken care of by the polling already done by the generic layer of ffado-mixer. Ultimately I don't think this is much more work than doing a device-specific thing. What do you think? > So it is surely simpler to consider that ffado-mixer itself knows that a > specific device requires such an update (as it knows a lot of things about > each specific device so as to develop a relevant interface for each one) > and do the job, including the update of its ffado-dbus interface. To a point, yes. However, given the nature of the functionality I would prefer it if it could be exposed in a more generic manner so the need for it is obvious. If it's buried in a specific device's module it could easily get lost. And as suggested above, I don't think there's a huge amount of extra work required. Regards jonathan |