8:26 a.m.
Hi Denis
Thank you for reviewing this series of patches. We shall submit new
versions according to your comments. Please find any other clarifying
points inline.
On Thu, 2010-12-09 at 09:36 +0100, ext Denis Kenzior wrote:
> +
> +static void voicecall_set_call_status(struct ofono_cdma_voicecall_manager *vc,
> + enum cdma_call_status status)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path;
> + const char *status_str;
> + enum cdma_call_status old_status;
> +
> + if (vc->status == status)
> + return;
> +
> + old_status = vc->status;
> +
> + vc->status = status;
> +
> + status_str = cdma_call_status_to_string(status);
> + path = voicecall_build_path(vc);
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> + "State", DBUS_TYPE_STRING,
> + &status_str);
> +
> + if (status == CDMA_CALL_STATUS_ACTIVE &&
> + old_status == CDMA_CALL_STATUS_DIALING) {
> + const char *timestr;
> +
> + vc->start_time = time(NULL);
> + timestr = time_to_str(&vc->start_time);
So in GSM we only timestamp the call once it moves to the active state.
Do you really want to timestamp it in the dialing stage?
I'm not sure what you mean here? We are carrying out the timestamp when
the status is active having previously been dialing in a similar way to
GSM.
In this patch, setting of the state to active is not supported yet due
to our current hardware limitations, so the time implementation has been
provided but is untested. If you would prefer us to remove the
timestamping altogether from this patch, we can do?
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> + "StartTime", DBUS_TYPE_STRING,
> + ×tr);
> + }
> +}
> +
> +static void voicecall_set_call_lineid(struct ofono_cdma_voicecall_manager *v)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path;
> + const char *lineid_str;
> +
> + path = voicecall_build_path(v);
> +
> + /* For MO calls, LineID is the dialed phone number */
> + lineid_str = cdma_phone_number_to_string(&v->phone_number);
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> + "LineIdentification",
> + DBUS_TYPE_STRING, &lineid_str);
> +}
> +
> +static void manager_dial_callback(const struct ofono_error *error, void *data)
> +{
> + struct ofono_cdma_voicecall_manager *vc = data;
> + DBusMessage *reply;
> +
> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> + reply = __ofono_error_failed(vc->pending);
> + } else {
> + const char *path = voicecall_build_path(vc);
what path are you returning here? This is not in the API proposal...
This was an oversight, thank you for catching it- I will remove this.
> +
> + reply = dbus_message_new_method_return(vc->pending);
> +
> + dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
> + DBUS_TYPE_INVALID);
> + }
In general the preferred style is:
if (foo) {
bar
} else
blah
> +
> + __ofono_dbus_pending_reply(&vc->pending, reply);
> +}
> +
> +static DBusMessage *voicecall_manager_dial(DBusConnection *conn,
> + DBusMessage *msg, void *data)
> +{
> + struct ofono_cdma_voicecall_manager *vc = data;
> + const char *number;
> +
> + if (vc->pending)
> + return __ofono_error_busy(msg);
> +
> + if (vc->status != CDMA_CALL_STATUS_DISCONNECTED)
> + return __ofono_error_failed(msg);
> +
> + if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> + DBUS_TYPE_INVALID) == FALSE)
> + return __ofono_error_invalid_args(msg);
> +
> + if (!valid_cdma_phone_number_format(number))
> + return __ofono_error_invalid_format(msg);
> +
> + if (vc->driver->dial == NULL)
> + return __ofono_error_not_implemented(msg);
> +
> + vc->pending = dbus_message_ref(msg);
> +
> + string_to_cdma_phone_number(number, &vc->phone_number);
> + voicecall_set_call_lineid(vc);
> + vc->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> + voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
> + vc->driver->dial(vc, &vc->phone_number, manager_dial_callback,
vc);
> +
What are the expectations on how the dial works?
e.g. in GSM we don't create the call until after the dial returns.
Normally this works this way:
---> ATD
<--- OK
State Dialing
State Alerting
State Connected
Does CDMA work in the same manner?
My other concern here is that you set the call status, line id, etc but
never reset them if the dial fails...
Thank you, yes CDMA dial does work in the same way as GSM essentially. I
shall move the call creation into the manager_dial_callback method, to
be executed upon a successful return from the dial.
Support for other call state support will come through with the future
work for call state transitions.
>
> +const char *cdma_phone_number_to_string(
> + const struct ofono_cdma_phone_number *ph)
> +{
> + static char buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH + 1];
> +
> + strncpy(buffer, ph->number, OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH);
> + buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH] = '\0';
> +
> + return buffer;
> +}
> +
> +void string_to_cdma_phone_number(const char *str,
> + struct ofono_cdma_phone_number *ph)
> +{
> + strcpy(ph->number, str);
Do we have international '+' prefix in CDMA?
Sometimes. If the CDMA networks support network based plus code dialing
then '+' can be used to replace the international direct dial prefix.
Our intention is to support international dialing with future work.
Cheers
Dara