Re: [PATCH] add anwser/send flash/send tones support for cdma voice call

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10460 bytes --]

Hi Caiwen,

Please use git-send-email to send your patches, and break them up
accordingly.  See 'Submitting patches' section in HACKING for a brief
introduction.

On 07/24/2011 10:24 PM, Caiwen Zhang wrote:
> diff --git a/include/cdma-voicecall.h b/include/cdma-voicecall.h
> index 9e741da..520934b 100644
> --- a/include/cdma-voicecall.h
> +++ b/include/cdma-voicecall.h
> @@ -55,8 +55,20 @@ struct ofono_cdma_voicecall_driver {
>  	/* Hangs up active, dialing, alerting or incoming calls */
>  	void (*hangup)(struct ofono_cdma_voicecall *vc,
>  			ofono_cdma_voicecall_cb_t cb, void *data);
> +
> +	void (*answer)(struct ofono_cdma_voicecall *vc,
> +			ofono_cdma_voicecall_cb_t cb, void *data);
> +
> +	void (*send_flash)(struct ofono_cdma_voicecall *vc, const char *string,
> +			ofono_cdma_voicecall_cb_t cb, void *data);
> +
> +	void (*send_tones)(struct ofono_cdma_voicecall *vc, const char *tones,
> +			ofono_cdma_voicecall_cb_t cb, void *data);
>  };
>  
> +void ofono_cdma_voicecall_notify(struct ofono_cdma_voicecall *vc,
> +	int direction, enum cdma_call_status status, const char *number);
> +
>  void ofono_cdma_voicecall_disconnected(struct ofono_cdma_voicecall *vc,
>  					enum ofono_disconnect_reason reason,
>  					const struct ofono_error *error);

This looks fine, but please break this up logically.  e.g.

- patch that introduces send_flash API in cdma-voicecall.h
- patch that implements it in the core
- patch that introduces send_tones API
- patch that implements it in the core
- patch that introduces notify / answer
- patch that implements them in the core

> diff --git a/src/cdma-voicecall.c b/src/cdma-voicecall.c
> index 183433d..20ab831 100644
> --- a/src/cdma-voicecall.c
> +++ b/src/cdma-voicecall.c
> @@ -40,6 +40,7 @@ static GSList *g_drivers;
>  
>  struct ofono_cdma_voicecall {
>  	struct ofono_cdma_phone_number phone_number;
> +	struct ofono_cdma_phone_number waiting_number;
>  	int direction;
>  	enum cdma_call_status status;
>  	time_t start_time;
> @@ -108,14 +109,33 @@ static void append_voicecall_properties(struct ofono_cdma_voicecall *vc,
>  {
>  	const char *status;
>  	const char *lineid;
> +	const char *waiting_call = NULL;
> +	ofono_bool_t call_waiting;
>  
>  	status = cdma_call_status_to_string(vc->status);
> -	lineid = cdma_phone_number_to_string(&vc->phone_number);
>  
>  	ofono_dbus_dict_append(dict, "State", DBUS_TYPE_STRING, &status);
>  
> -	ofono_dbus_dict_append(dict, "LineIdentification",
> -				DBUS_TYPE_STRING, &lineid);
> +	if (vc->status != CDMA_CALL_STATUS_DISCONNECTED) {
> +		if (vc->phone_number.number[0] != '\0') {
> +			lineid = cdma_phone_number_to_string(&vc->phone_number);
> +			ofono_dbus_dict_append(dict, "LineIdentification",
> +						DBUS_TYPE_STRING, &lineid);
> +		}
> +
> +		if (vc->waiting_number.number[0] != '\0') {
> +			waiting_call = cdma_phone_number_to_string(
> +						&vc->waiting_number);
> +
> +			ofono_dbus_dict_append(dict, "CallWaitingNumber",
> +					DBUS_TYPE_STRING, &waiting_call);
> +		}
> +	}
> +
> +	call_waiting = (waiting_call != NULL);
> +
> +	ofono_dbus_dict_append(dict, "CallWaiting",
> +					DBUS_TYPE_BOOLEAN, &call_waiting);
>  

Why do you put the CallWaitingNumber handling in the if above, but not
the CallWaiting property?

>  	if (vc->status == CDMA_CALL_STATUS_ACTIVE) {
>  		const char *timestr = time_to_str(&vc->start_time);
> @@ -172,6 +192,7 @@ static void voicecall_set_call_status(struct ofono_cdma_voicecall *vc,
>  	const char *status_str;
>  	enum cdma_call_status old_status;
>  
> +	DBG("status: %s", cdma_call_status_to_string(status));
>  	if (vc->status == status)
>  		return;
>  
> @@ -198,6 +219,14 @@ static void voicecall_set_call_status(struct ofono_cdma_voicecall *vc,
>  					"StartTime", DBUS_TYPE_STRING,
>  					&timestr);
>  	}
> +
> +	if (status == CDMA_CALL_STATUS_DISCONNECTED) {
> +		memset(&vc->phone_number, 0,
> +				sizeof(struct ofono_cdma_phone_number));
> +
> +		memset(&vc->waiting_number, 0,
> +			sizeof(struct ofono_cdma_phone_number));
> +	}

What happens to the waiting call?

>  }
>  
>  static void voicecall_set_call_lineid(struct ofono_cdma_voicecall *vc)
> @@ -215,6 +244,32 @@ static void voicecall_set_call_lineid(struct ofono_cdma_voicecall *vc)
>  					DBUS_TYPE_STRING, &lineid_str);
>  }
>  
> +static void voicecall_set_call_waiting_number(struct ofono_cdma_voicecall *vc,
> +				const struct ofono_cdma_phone_number *ph)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path  = __ofono_atom_get_path(vc->atom);
> +	const char *waiting_number;
> +	ofono_bool_t call_waiting = TRUE;
> +
> +	if (ph->number[0] == '\0')
> +		return;
> +
> +	memcpy(&vc->waiting_number, ph, sizeof(struct ofono_cdma_phone_number));
> +
> +	waiting_number = cdma_phone_number_to_string(ph);
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					"CallWaitingNumber", DBUS_TYPE_STRING,
> +					&waiting_number);
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					"CallWaiting", DBUS_TYPE_BOOLEAN,
> +					&call_waiting);
> +}
> +
>  static void manager_dial_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_cdma_voicecall *vc = data;
> @@ -229,7 +284,9 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
>  
>  	voicecall_set_call_lineid(vc);
>  	vc->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> -	voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
> +
> +	if (vc->status != CDMA_CALL_STATUS_ACTIVE)
> +		voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
>  
>  	reply = dbus_message_new_method_return(vc->pending);
>  	__ofono_dbus_pending_reply(&vc->pending, reply);
> @@ -286,6 +343,105 @@ static DBusMessage *voicecall_manager_hangup(DBusConnection *conn,
>  	return NULL;
>  }
>  
> +static DBusMessage *voicecall_manager_answer(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall *vc = data;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (vc->driver->answer == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	if (vc->status != CDMA_CALL_STATUS_INCOMING)
> +		return __ofono_error_failed(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	vc->driver->answer(vc, generic_callback, vc);
> +
> +	return NULL;
> +}
> +
> +static DBusMessage *voicecall_manager_flash(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall *vc = data;
> +	const char *string;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (vc->driver->send_flash == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &string,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (vc->status == CDMA_CALL_STATUS_DISCONNECTED)
> +		return __ofono_error_failed(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	vc->driver->send_flash(vc, string, generic_callback, vc);
> +
> +	return NULL;
> +}
> +
> +static ofono_bool_t is_valid_tones(const char *tones)
> +{
> +	int len;
> +	int i;
> +
> +	if (tones == NULL)
> +		return FALSE;
> +
> +	len = strlen(tones);
> +	if (len == 0)
> +		return FALSE;
> +
> +	for (i = 0; i < len; i++) {
> +		if (g_ascii_isdigit(tones[i]) || tones[i] == '*' ||
> +				tones[i] == '#')
> +			continue;
> +		else
> +			return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +static DBusMessage *voicecall_manager_tone(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall *vc = data;
> +	const char *tones;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (vc->driver->send_tones == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	if (vc->status != CDMA_CALL_STATUS_ACTIVE)
> +		return __ofono_error_failed(msg);
> +
> +	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &tones,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (is_valid_tones(tones) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	vc->driver->send_tones(vc,  tones, generic_callback, vc);
> +
> +	return NULL;
> +}
> +
>  static GDBusMethodTable manager_methods[] = {
>  	{ "GetProperties",    "",    "a{sv}",
>  					voicecall_manager_get_properties },
> @@ -293,6 +449,12 @@ static GDBusMethodTable manager_methods[] = {
>  						G_DBUS_METHOD_FLAG_ASYNC },
>  	{ "Hangup",           "",    "",         voicecall_manager_hangup,
>  						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "Answer",           "",    "",         voicecall_manager_answer,
> +						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "SendFlash",      "s",    "",         voicecall_manager_flash,
> +						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "SendTones",     "s",    "",        voicecall_manager_tone,
> +						G_DBUS_METHOD_FLAG_ASYNC },
>  	{ }
>  };
>  
> @@ -302,6 +464,31 @@ static GDBusSignalTable manager_signals[] = {
>  	{ }
>  };
>  
> +void ofono_cdma_voicecall_notify(struct ofono_cdma_voicecall *vc,
> +			int direction, enum cdma_call_status status,
> +			const char *number)
> +{
> +	struct ofono_cdma_phone_number ph;
> +
> +	DBG("direction: %d; status: %d; num: %s", direction, status, number);
> +
> +	string_to_cdma_phone_number(number, &ph);
> +
> +	if (status == CDMA_CALL_STATUS_INCOMING
> +			&& vc->status == CDMA_CALL_STATUS_ACTIVE) {
> +		voicecall_set_call_waiting_number(vc, &ph);

Would it be a good idea to formalize the waiting state?

> +	} else {
> +		memcpy(&vc->phone_number, &ph,
> +				sizeof(struct ofono_cdma_phone_number));
> +
> +		voicecall_set_call_lineid(vc);
> +	}
> +
> +	vc->direction = direction;
> +
> +	voicecall_set_call_status(vc, status);
> +}
> +
>  void ofono_cdma_voicecall_disconnected(struct ofono_cdma_voicecall *vc,
>  					enum ofono_disconnect_reason reason,
>  					const struct ofono_error *error)

I'd also like to see some cdma-voicecall drivers in order to visualize
what is going on better.

Regards,
-Denis