HFP: typo in error path?

HFP: typo in error path?

[Daniel Wagner]

Hi,

I'm reading a bit through the code and try to understand how the HF
role in HFP is implemented. I found following in audio/gateway.c:

static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
			struct audio_device *dev)
{
	struct gateway *gw = dev->gateway;

	if (cond & G_IO_NVAL)
		return FALSE;

	if (cond & (G_IO_ERR | G_IO_HUP)) {
		DBG("sco connection is released");
		g_io_channel_shutdown(gw->sco, TRUE, NULL);
		g_io_channel_unref(gw->sco);
		gw->sco = NULL;
		change_state(dev, GATEWAY_STATE_CONNECTED);
		return FALSE;
	}

	return TRUE;
}

I don't really understand what's going on here, but just from the
naming I think the change_state call should be
GATEWAY_STATE_DISCONNECTED. If my assumation is correct I can spin a
patch.

cheers,
daniel

Re: HFP: typo in error path?

[Johan Hedberg]

Hi Daniel,

On Thu, Dec 16, 2010, Daniel Wagner wrote:
> I'm reading a bit through the code and try to understand how the HF
> role in HFP is implemented. I found following in audio/gateway.c:
> 
> static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
> 			struct audio_device *dev)
> {
> 	struct gateway *gw = dev->gateway;
> 
> 	if (cond & G_IO_NVAL)
> 		return FALSE;
> 
> 	if (cond & (G_IO_ERR | G_IO_HUP)) {
> 		DBG("sco connection is released");
> 		g_io_channel_shutdown(gw->sco, TRUE, NULL);
> 		g_io_channel_unref(gw->sco);
> 		gw->sco = NULL;
> 		change_state(dev, GATEWAY_STATE_CONNECTED);
> 		return FALSE;
> 	}
> 
> 	return TRUE;
> }
> 
> I don't really understand what's going on here, but just from the
> naming I think the change_state call should be
> GATEWAY_STATE_DISCONNECTED. If my assumation is correct I can spin a
> patch.

Actually the current code is correct. This is the callback for SCO (like
the function name suggests) so when SCO gets closed there's a transition
from PLAYING to CONNECTED (meaning RFCOMM but no SCO). You can see the
full set of state values in audio/gateway.h.

Johan

Re: HFP: typo in error path?

[Daniel Wagner]

Hi Johan,

On Thu, Dec 16, 2010 at 09:10:15AM +0200, Johan Hedberg wrote:
> > I'm reading a bit through the code and try to understand how the HF
> > role in HFP is implemented. I found following in audio/gateway.c:
> > 
> > static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
> > 			struct audio_device *dev)
> > {
> > 	struct gateway *gw = dev->gateway;
> > 
> > 	if (cond & G_IO_NVAL)
> > 		return FALSE;
> > 
> > 	if (cond & (G_IO_ERR | G_IO_HUP)) {
> > 		DBG("sco connection is released");
> > 		g_io_channel_shutdown(gw->sco, TRUE, NULL);
> > 		g_io_channel_unref(gw->sco);
> > 		gw->sco = NULL;
> > 		change_state(dev, GATEWAY_STATE_CONNECTED);
> > 		return FALSE;
> > 	}
> > 
> > 	return TRUE;
> > }
> > 
> > I don't really understand what's going on here, but just from the
> > naming I think the change_state call should be
> > GATEWAY_STATE_DISCONNECTED. If my assumation is correct I can spin a
> > patch.
> 
> Actually the current code is correct. This is the callback for SCO (like
> the function name suggests) so when SCO gets closed there's a transition
> from PLAYING to CONNECTED (meaning RFCOMM but no SCO). You can see the
> full set of state values in audio/gateway.h.

Thanks for the clarification, that make perfectly sense now :)

cheers,
daniel

Re: HFP: typo in error path?

[Gustavo F. Padovan]

Hi Daniel,

* Daniel Wagner <wagi@monom.org> [2010-12-16 07:42:31 +0100]:

> Hi,
> 
> I'm reading a bit through the code and try to understand how the HF
> role in HFP is implemented. I found following in audio/gateway.c:
> 
> static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
> 			struct audio_device *dev)
> {
> 	struct gateway *gw = dev->gateway;
> 
> 	if (cond & G_IO_NVAL)
> 		return FALSE;
> 
> 	if (cond & (G_IO_ERR | G_IO_HUP)) {
> 		DBG("sco connection is released");
> 		g_io_channel_shutdown(gw->sco, TRUE, NULL);
> 		g_io_channel_unref(gw->sco);
> 		gw->sco = NULL;
> 		change_state(dev, GATEWAY_STATE_CONNECTED);
> 		return FALSE;
> 	}
> 
> 	return TRUE;
> }
> 
> I don't really understand what's going on here, but just from the
> naming I think the change_state call should be
> GATEWAY_STATE_DISCONNECTED. If my assumation is correct I can spin a
> patch.

GATEWAY_STATE_CONNECTED means that we have Service Level Connection with the
Phone, then if a new call arrives we change to the GATEWAY_STATE_PLAYING
state to signal that we have audio, but if for some reason the SCO link fails
we just go back to the GATEWAY_STATE_CONNECTED state because we still have the
SLC running. So your assumption is wrong. ;)

-- 
Gustavo F. Padovan
http://profusion.mobi