* [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support @ 2010-12-22 0:02 Lei Yu 2011-01-04 21:17 ` Denis Kenzior 2011-01-05 22:34 ` Rajesh.Nagaiah 0 siblings, 2 replies; 6+ messages in thread From: Lei Yu @ 2010-12-22 0:02 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 22544 bytes --] --- Makefile.am | 3 +- src/cdma-smsutil.c | 484 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/cdma-smsutil.h | 264 ++++++++++++++++++++++++++++ 3 files changed, 750 insertions(+), 1 deletions(-) create mode 100644 src/cdma-smsutil.c create mode 100644 src/cdma-smsutil.h diff --git a/Makefile.am b/Makefile.am index aa00016..e85f522 100644 --- a/Makefile.am +++ b/Makefile.am @@ -319,7 +319,8 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \ src/radio-settings.c src/stkutil.h src/stkutil.c \ src/nettime.c src/stkagent.c src/stkagent.h \ src/simfs.c src/simfs.h src/audio-settings.c \ - src/smsagent.c src/smsagent.h src/ctm.c + src/smsagent.c src/smsagent.h src/ctm.c \ + src/cdma-smsutil.c src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl diff --git a/src/cdma-smsutil.c b/src/cdma-smsutil.c new file mode 100644 index 0000000..b80002c --- /dev/null +++ b/src/cdma-smsutil.c @@ -0,0 +1,484 @@ +/* + * + * oFono - Open Source Telephony + * + * Copyright (C) 2010 Nokia Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#ifdef HAVE_CONFIG_H +#include <config.h> +#endif + +#define _GNU_SOURCE +#include <string.h> +#include <dirent.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <glib.h> + +#include "cdma-smsutil.h" + +/* + * Mapping from binary DTMF code to the digit it represents. + * As defined in Table 2.7.1.3.2.4-4 of 3GPP2 C.S0005-E v2.0. + * Note, 0 is NOT a valid value and not mapped to + * any valid DTMF digit. + */ +const char cdma_sms_dtmf_digits[13] = {0, '1', '2', '3', '4', '5', '6', '7', + '8', '9', '0', '*', '#'}; + + +/* Unpacks the byte stream. */ +static guint32 bit_field_unpack(const guint8 *buf, guint32 offset, + guint32 nbit) +{ + guint32 val = 0; + guint32 byteIndex; + guint32 remainder; + + if (buf == NULL) + return 0; + + byteIndex = offset >> 3; + remainder = (offset & 0x7); + + if (remainder != 0) { + /* + * The field to be unpacked does not start at the byte + * boundary. Retrieve those bits first. + */ + guint32 mask; + + mask = (1 << (8 - remainder)) - 1; + val = buf[byteIndex] & mask; + + /* The field is within current byte */ + if (nbit < (8 - remainder)) + return (val >> (8 - remainder - nbit)); + + /* The field at least spans till end of the byte. */ + nbit -= (8 - remainder); + byteIndex++; + } + + /* Unpack rest of the bits in the field in 8-bit chunk. */ + while (nbit >= 8) { + val = (val << 8) | buf[byteIndex]; + nbit -= 8; + byteIndex++; + } + + /* If still some left, unpack the last remaining bits. */ + if (nbit > 0) + val = (val << nbit) | (buf[byteIndex] >> (8 - nbit)); + + return val; +} + +/* Convert CDMA DTMF digits into a string */ +static gboolean cdma_sms_dtmf_to_ascii(char *buf, const char *addr, + guint8 num_fields) +{ + guint8 index; + guint8 value; + + if (buf == NULL || addr == NULL) + return FALSE; + + for (index = 0; index < num_fields; index++) { + if ((addr[index] <= 0) || (addr[index] > 12)) + return FALSE; /* Invalid digit in address field */ + + value = (guint8) addr[index]; + buf[index] = cdma_sms_dtmf_digits[value]; + } + + buf[index] = 0; /* Make it NULL terminated string */ + + return TRUE; +} + +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr) +{ + char *buf; + + if (addr == NULL) + return NULL; + + if (addr->digit_mode != DTMF_4_BIT) + return NULL; /* TODO: Only support DTMF_4_BIT currently */ + + buf = g_new(char, addr->num_fields + 1); + if (buf == NULL) + return NULL; + + if (!cdma_sms_dtmf_to_ascii(buf, addr->address, addr->num_fields)) { + g_free(buf); + return NULL; + } + + return buf; +} + +/* Decode Address parameter record */ +static gboolean cdma_sms_decode_addr(const unsigned char *buf, int len, + struct cdma_sms_address *addr) +{ + guint32 bit_offset = 0; + guint8 chari_len; + guint32 total_num_bits = len * 8; + guint8 index; + + if (len <= 0 || buf == NULL || addr == NULL) + return FALSE; + + addr->digit_mode = (enum cdma_sms_digit_mode) + bit_field_unpack(buf, 0, 1); + bit_offset += 1; + addr->number_mode = (guint8) bit_field_unpack(buf, bit_offset, 1); + bit_offset += 1; + + if (addr->digit_mode == CODE_8_BIT) { + addr->number_type = (guint8) + bit_field_unpack(buf, bit_offset, 3); + bit_offset += 3; + + if (addr->number_mode == 0) { + if (bit_offset + 4 > total_num_bits) + return FALSE; + + addr->number_plan = (enum cdma_sms_numbering_plan) + bit_field_unpack(buf, bit_offset, 4); + bit_offset += 4; + } + } + + if ((bit_offset + 8) > total_num_bits) + return FALSE; + + addr->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); + bit_offset += 8; + + if (addr->digit_mode == DTMF_4_BIT) + chari_len = 4; + else + chari_len = 8; + + if ((bit_offset + chari_len * addr->num_fields) > total_num_bits) + return FALSE; + + for (index = 0; index < addr->num_fields; index++) { + addr->address[index] = (char) bit_field_unpack(buf, + bit_offset, + chari_len); + bit_offset += chari_len; + } + + return TRUE; +} + +char *cdma_sms_decode_text(struct cdma_sms_ud *ud) +{ + char *buf; + + if (ud == NULL) + return NULL; + + /* TODO: Only support MSG_ENCODING_7BIT_ASCII currently */ + if (ud->msg_encoding != MSG_ENCODING_7BIT_ASCII) + return NULL; + + buf = g_new(char, ud->num_fields + 1); + if (buf == NULL) + return NULL; + + memcpy(buf, ud->chari, ud->num_fields); + buf[ud->num_fields] = 0; /* Make it NULL terminated string */ + + return buf; +} + +/* Decode User Data */ +static gboolean cdma_sms_decode_ud(const unsigned char *buf, int len, + struct cdma_sms_ud *ud) +{ + guint32 bit_offset = 0; + guint8 chari_len = 0; + guint32 total_num_bits = len * 8; + guint8 index; + enum cdma_sms_message_encoding msg_encoding; + + if (buf == NULL || ud == NULL) + return FALSE; + + if (total_num_bits < 13) + return FALSE; + + msg_encoding = (enum cdma_sms_message_encoding) + bit_field_unpack(buf, bit_offset, 5); + ud->msg_encoding = msg_encoding; + bit_offset += 5; + + if (ud->msg_encoding == MSG_ENCODING_EXTENDED_PROTOCOL_MSG || + ud->msg_encoding == MSG_ENCODING_GSM_DATA_CODING) { + /* Skip message type field */ + /* TODO: Add support for message type field */ + bit_offset += 8; + } + + if (bit_offset + 8 > total_num_bits) + return FALSE; + + ud->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); + bit_offset += 8; + + switch (msg_encoding) { + case MSG_ENCODING_OCTET: + chari_len = 8; + break; + case MSG_ENCODING_EXTENDED_PROTOCOL_MSG: + return FALSE; /* TODO */ + case MSG_ENCODING_7BIT_ASCII: + case MSG_ENCODING_IA5: + chari_len = 7; + break; + case MSG_ENCODING_UNICODE: + case MSG_ENCODING_SHIFT_JIS: + case MSG_ENCODING_KOREAN: + return FALSE; /* TODO */ + case MSG_ENCODING_LATIN_HEBREW: + case MSG_ENCODING_LATIN: + chari_len = 8; + break; + case MSG_ENCODING_GSM_7BIT: + chari_len = 7; + break; + case MSG_ENCODING_GSM_DATA_CODING: + return FALSE; /* TODO */ + } + + if (chari_len == 0) + return FALSE; + + if (bit_offset + chari_len * ud->num_fields > total_num_bits) + return FALSE; + + for (index = 0; index < ud->num_fields; index++) { + ud->chari[index] = (guint8) bit_field_unpack(buf, + bit_offset, + chari_len); + bit_offset += chari_len; + } + + return TRUE; +} + +/* Decode Message Identifier */ +static gboolean cdma_sms_decode_message_id(const unsigned char *buf, int len, + struct cdma_sms_identifier *id) +{ + guint32 bit_offset = 0; + + if (buf == NULL || id == NULL) + return FALSE; + + if (len != 3) + return FALSE; + + id->msg_type = (enum cdma_sms_msg_type) + bit_field_unpack(buf, bit_offset, 4); + bit_offset += 4; + id->msg_id = (guint16) bit_field_unpack(buf, bit_offset, 16); + bit_offset += 16; + id->header_ind = (gboolean) bit_field_unpack(buf, bit_offset, 1); + + return TRUE; +} + +/* Decode Bearer Data */ +static gboolean cdma_sms_decode_bearer_data(const unsigned char *buf, int len, + struct cdma_sms_bearer_data *bearer_data) +{ + gboolean ret = TRUE; + enum cdma_sms_subparam_id subparam_id; + guint8 subparam_len; + + if (buf == NULL || bearer_data == NULL) + return FALSE; + + while (len != 0) { + + if (len < 2) + return FALSE; + + subparam_id = (enum cdma_sms_subparam_id) + bit_field_unpack(buf, 0, 8); + buf += 1; + subparam_len = (guint8) bit_field_unpack(buf, 0, 8); + buf += 1; + + len -= 2; + + if (len < subparam_len) + return FALSE; + + switch (subparam_id) { + case CDMA_SMS_SUBPARAM_ID_MESSAGE_ID: + ret = cdma_sms_decode_message_id(buf, subparam_len, + &bearer_data->id); + set_bitmap(&bearer_data->subparam_bitmap, + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID); + break; + case CDMA_SMS_SUBPARAM_ID_USER_DATA: + ret = cdma_sms_decode_ud(buf, subparam_len, + &bearer_data->ud); + set_bitmap(&bearer_data->subparam_bitmap, + CDMA_SMS_SUBPARAM_ID_USER_DATA); + break; + case CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE: + case CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP: + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE: + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE: + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE: + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE: + case CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR: + case CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR: + case CDMA_SMS_SUBPARAM_ID_REPLY_OPTION: + case CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES: + case CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY: + case CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR: + case CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER: + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE: + case CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA: + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX: + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA: + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT: + case CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS: + case CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE: + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN: + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK: + /* + * TODO: Ignore any unsupported parameter. + * Also ignore any un-recognized parameter for + * future standard revision. + */ + break; + } + + if (ret == FALSE) + return FALSE; + + len -= subparam_len; + buf += subparam_len; + } + + return TRUE; +} + +static gboolean cdma_sms_p2p_decode(const unsigned char *pdu, int len, + struct cdma_sms *incoming) +{ + gboolean ret = TRUE; + enum cdma_sms_param_id rec_id; + guint8 rec_len; + + if (pdu == NULL || incoming == NULL) + return FALSE; + + while (len != 0) { + + if (len <= 2) + return FALSE; + + rec_id = (enum cdma_sms_param_id) bit_field_unpack(pdu, 0, 8); + pdu += 1; + rec_len = (guint8) bit_field_unpack(pdu, 0, 8); + pdu += 1; + + len -= 2; + + if (len < rec_len) + return FALSE; + + switch (rec_id) { + case CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER: + incoming->p2p_msg.teleservice_id = + (enum cdma_sms_teleservice_id) + bit_field_unpack(pdu, 0, 16); + set_bitmap(&incoming->p2p_msg.param_bitmap, + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER); + break; + case CDMA_SMS_PARAM_ID_SERVICE_CATEGORY: + break; /* TODO */ + case CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS: + ret = cdma_sms_decode_addr(pdu, rec_len, + &incoming->p2p_msg.oaddr); + set_bitmap(&incoming->p2p_msg.param_bitmap, + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS); + break; + case CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS: + case CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS: + case CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS: + case CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION: + case CDMA_SMS_PARAM_ID_CAUSE_CODE: + break; /* TODO */ + case CDMA_SMS_PARAM_ID_BEARER_DATA: + ret = cdma_sms_decode_bearer_data(pdu, rec_len, + &incoming->p2p_msg.bearer_data); + set_bitmap(&incoming->p2p_msg.param_bitmap, + CDMA_SMS_PARAM_ID_BEARER_DATA); + break; + } + + if (!ret) + return FALSE; + + len -= rec_len; + pdu += rec_len; + } + + return TRUE; +} + +gboolean cdma_sms_decode(const unsigned char *pdu, int len, + struct cdma_sms *incoming) +{ + gboolean ret = FALSE; + + if ((len == 0) || (pdu == NULL) || (incoming == NULL)) + return FALSE; + + incoming->type = (enum cdma_sms_tp_msg_type) + bit_field_unpack(pdu, 0, 8); + pdu += 1; + len -= 1; + + switch (incoming->type) { + case CDMA_SMS_P2P: + ret = cdma_sms_p2p_decode(pdu, len, incoming); + break; + case CDMA_SMS_BCAST: + case CDMA_SMS_ACK: + /* TODO: Not supported yet */ + ret = FALSE; + break; + } + + return ret; +} diff --git a/src/cdma-smsutil.h b/src/cdma-smsutil.h new file mode 100644 index 0000000..bfe2abb --- /dev/null +++ b/src/cdma-smsutil.h @@ -0,0 +1,264 @@ +/* + * + * oFono - Open Source Telephony + * + * Copyright (C) 2010 Nokia Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +/* 3GPP2 C.S0015-B v2.0, Table 3.4-1 */ +enum cdma_sms_tp_msg_type { + CDMA_SMS_P2P = 0, + CDMA_SMS_BCAST = 1, + CDMA_SMS_ACK = 2 +}; + +/* 3GPP2 X.S0004-550-E, Section 2.256 */ +enum cdma_sms_teleservice_id { + TELESERVICE_CMT91 = 4096, + TELESERVICE_WPT = 4097, + TELESERVICE_WMT = 4098, + TELESERVICE_VMN = 4099, + TELESERVICE_WAP = 4100, + TELESERVICE_WEMT = 4101, + TELESERVICE_SCPT = 4102, + TELESERVICE_CATPT = 4103 +}; + +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ +enum cdma_sms_digi_number_type { + CDMA_SMS_NUM_TYPE_UNKNOWN = 0, + CDMA_SMS_NUM_TYPE_INTERNATIONAL_NUMBER = 1, + CDMA_SMS_NUM_TYPE_NATIONAL_NUMBER = 2, + CDMA_SMS_NUM_TYPE_NETWORK_SPECIFIC_NUMBER = 3, + CDMA_SMS_NUM_TYPE_SUBSCRIBER_NUMBER = 4, + /* Reserved 5 */ + CDMA_SMS_NUM_TYPE_ABBREVIATED_NUMBER = 6 + /* Reserved 7 */ +}; + +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ +enum cdma_sms_data_network_number_type { + CDMA_SMS_NETWORK_NUM_TYPE_UNKNOWN = 0, + CDMA_SMS_NETWORK_TYPE_INTERNET_PROTOCOL = 1, + CDMA_SMS_NETWORK_TYPE_INTERNET_EMAIL_ADDRESS = 2, + /* All Other Values Reserved */ +}; + +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-3 */ +enum cdma_sms_numbering_plan { + CDMA_SMS_NUMBERING_PLAN_UNKNOWN = 0, + CDMA_SMS_NUMBERING_PLAN_ISDN = 1, + CDMA_SMS_NUMBERING_PLAN_DATA = 3, + CDMA_SMS_NUMBERING_PLAN_TELEX = 4, + CDMA_SMS_NUMBERING_PLAN_PRIVATE = 9, + CDMA_SMS_NUMBERING_PLAN_RESERVED = 15 +}; + +/* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ +enum cdma_sms_msg_type { + CDMA_SMS_RESERVED = 0, + CDMA_SMS_DELIVER = 1, + CDMA_SMS_SUBMIT = 2, + CDMA_SMS_CANCEL = 3, + CDMA_SMS_DELIVER_ACK = 4, + CDMA_SMS_USER_ACK = 5, + CDMA_SMS_READ_ACK = 6, +}; + +/* C.R1001-G_v1.0 Table 9.1-1 */ +enum cdma_sms_message_encoding { + MSG_ENCODING_OCTET = 0, + MSG_ENCODING_EXTENDED_PROTOCOL_MSG = 1, + MSG_ENCODING_7BIT_ASCII = 2, + MSG_ENCODING_IA5 = 3, + MSG_ENCODING_UNICODE = 4, + MSG_ENCODING_SHIFT_JIS = 5, + MSG_ENCODING_KOREAN = 6, + MSG_ENCODING_LATIN_HEBREW = 7, + MSG_ENCODING_LATIN = 8, + MSG_ENCODING_GSM_7BIT = 9, + MSG_ENCODING_GSM_DATA_CODING = 10 +}; + +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3-1 */ +enum cdma_sms_param_id { + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER = 0x00, + CDMA_SMS_PARAM_ID_SERVICE_CATEGORY = 0x01, + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS = 0x02, + CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS = 0x03, + CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS = 0x04, + CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS = 0x05, + CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION = 0x06, + CDMA_SMS_PARAM_ID_CAUSE_CODE = 0x07, + CDMA_SMS_PARAM_ID_BEARER_DATA = 0x08 +}; + +/* 3GPP2 C.S0015-B v2.0 Table 4.5-1 */ +enum cdma_sms_subparam_id { + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID = 0x00, + CDMA_SMS_SUBPARAM_ID_USER_DATA = 0x01, + CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE = 0x02, + CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP = 0x03, + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE = 0x04, + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE = 0x05, + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE = 0x06, + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE = 0x07, + CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR = 0x08, + CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR = 0x09, + CDMA_SMS_SUBPARAM_ID_REPLY_OPTION = 0x0A, + CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES = 0x0B, + CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY = 0x0C, + CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR = 0x0D, + CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER = 0x0E, + CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE = 0x0F, + CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA = 0x10, + CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX = 0x11, + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA = 0x12, + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT = 0x13, + CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS = 0x14, + CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE = 0x15, + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN = 0x16, + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK = 0x17 +}; + +/* 3GPP2 C.R1001-G Table 9.3.1-1 */ +enum cdma_sms_service_category { + SERVICE_CAT_EMERGENCY_BROADCAST = 1, + SERVICE_CAT_ADMINISTRATIVE = 2, + SERVICE_CAT_MAINTENANCE = 3, + SERVICE_CAT_GENERALNEWSLOCAL = 4, + SERVICE_CAT_GENERALNEWSREGIONAL = 5, + SERVICE_CAT_GENERALNEWSNATIONAL = 6, + SERVICE_CAT_GENERALNEWSINTERNATIONAL = 7, + SERVICE_CAT_BUSINESSFINALNEWSLOCAL = 8, + SERVICE_CAT_BUSINESSFINALNEWSREGIONAL = 9, + SERVICE_CAT_BUSINESSFINALNEWSNATIONAL = 10, + SERVICE_CAT_BUSINESSFINALNEWSINTNL = 11, + SERVICE_CAT_SPORTSNEWSLOCAL = 12, + SERVICE_CAT_SPORTSNEWSREGIONAL = 13, + SERVICE_CAT_SPORTSNEWSNATIONAL = 14, + SERVICE_CAT_SPORTSNEWSINTERNATIONAL = 15, + SERVICE_CAT_ENTERTAINMENTNEWSLOCAL = 16, + SERVICE_CAT_ENTERTAINMENTNEWSREGIONAL = 17, + SERVICE_CAT_ENTERTAINMENTNEWSNATIONAL = 18, + SERVICE_CAT_ENTERTAINMENTNEWSINTERNATIONAL = 19, + SERVICE_CAT_ENTERTAINMENTNEWSLOCALWEATHER = 20, + SERVICE_CAT_AREATRAFFICREPORTS = 21, + SERVICE_CAT_LOCALAIRTPORTFLIGHTSCHEDULES = 22, + SERVICE_CAT_RESTURANTS = 23, + SERVICE_CAT_LODGINGS = 24, + SERVICE_CAT_RETAILDIRECTORYADVERTISEMENTS = 25, + SERVICE_CAT_ADVERTISEMENTS = 26, + SERVICE_CAT_STOCKQUOTES = 27, + SERVICE_CAT_EMPLOYMENTOPPORTUNITIES = 28, + SERVICE_CAT_MEDICALHEALTHHOSPITALS = 29, + SERVICE_CAT_TECHNOLOGYNEWS = 30, + SERVICE_CAT_MULTICATEGORY = 31, + SERVICE_CAT_CAPT = 32 +}; + +enum cdma_sms_digit_mode { + DTMF_4_BIT = 0, + CODE_8_BIT = 1 +}; + +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ +struct cdma_sms_address { + enum cdma_sms_digit_mode digit_mode; + guint8 number_mode; + guint8 number_type; + enum cdma_sms_numbering_plan number_plan; + guint8 num_fields; + char address[256]; +}; + +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.6 */ +struct cdma_sms_cause_code { + guint8 reply_seq; + guint8 error_class; + guint8 cause_code; +}; + +/* 3GPP2 C.S0015-B v2.0 Section 4.5.1 */ +struct cdma_sms_identifier { + enum cdma_sms_msg_type msg_type; + guint16 msg_id; + gboolean header_ind; +}; + +/* 3GPP2 C.S0015-B v2.0 Section 4.5.2 */ +struct cdma_sms_ud { + enum cdma_sms_message_encoding msg_encoding; + guint8 num_fields; + guint8 chari[512]; +}; + +/* 3GPP2 C.S0015-B v2.0 Section 4.5 */ +struct cdma_sms_bearer_data { + guint32 subparam_bitmap; + struct cdma_sms_identifier id; + struct cdma_sms_ud ud; +}; + +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.1-1. */ +struct cdma_sms_p2p_msg { + guint32 param_bitmap; + enum cdma_sms_teleservice_id teleservice_id; + struct cdma_sms_address oaddr; + struct cdma_sms_bearer_data bearer_data; +}; + +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.2-1 */ +struct cdma_sms_broadcast_msg { + enum cdma_sms_service_category service_category; + struct cdma_sms_bearer_data bearer_data; +}; + +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.3-1 */ +struct cdma_sms_ack_msg { + struct cdma_sms_address daddr; + struct cdma_sms_cause_code cause_code; +}; + +/* 3GPP2 C.S0015-B v2.0 Section 3.4.1 */ +struct cdma_sms { + enum cdma_sms_tp_msg_type type; + union { + struct cdma_sms_p2p_msg p2p_msg; + struct cdma_sms_broadcast_msg broadcast_msg; + struct cdma_sms_ack_msg ack_msg; + }; +}; + +static inline gboolean check_bitmap(guint32 bitmap, guint32 pos) +{ + guint32 mask = 0x1 << pos; + return bitmap & mask ? TRUE : FALSE; +} + +static inline void set_bitmap(guint32 *bitmap, guint32 pos) +{ + if (bitmap == NULL) + return; + + *bitmap = *bitmap | (1 << pos); +} + +gboolean cdma_sms_decode(const unsigned char *pdu, int len, + struct cdma_sms *out); +char *cdma_sms_decode_text(struct cdma_sms_ud *ud); +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support 2010-12-22 0:02 [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support Lei Yu @ 2011-01-04 21:17 ` Denis Kenzior 2011-01-05 17:47 ` Lei Yu 2011-01-05 22:34 ` Rajesh.Nagaiah 1 sibling, 1 reply; 6+ messages in thread From: Denis Kenzior @ 2011-01-04 21:17 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 25870 bytes --] Hi Lei, On 12/21/2010 06:02 PM, Lei Yu wrote: > --- > Makefile.am | 3 +- > src/cdma-smsutil.c | 484 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/cdma-smsutil.h | 264 ++++++++++++++++++++++++++++ > 3 files changed, 750 insertions(+), 1 deletions(-) > create mode 100644 src/cdma-smsutil.c > create mode 100644 src/cdma-smsutil.h > > diff --git a/Makefile.am b/Makefile.am > index aa00016..e85f522 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -319,7 +319,8 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \ > src/radio-settings.c src/stkutil.h src/stkutil.c \ > src/nettime.c src/stkagent.c src/stkagent.h \ > src/simfs.c src/simfs.h src/audio-settings.c \ > - src/smsagent.c src/smsagent.h src/ctm.c > + src/smsagent.c src/smsagent.h src/ctm.c \ > + src/cdma-smsutil.c Make sure to include src/cdma-smsutil.h as well. I'm still not really familiar with the CDMA SMS pdu spec, so take my comments with a grain of salt ;) > > src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl > > diff --git a/src/cdma-smsutil.c b/src/cdma-smsutil.c > new file mode 100644 > index 0000000..b80002c > --- /dev/null > +++ b/src/cdma-smsutil.c > @@ -0,0 +1,484 @@ > +/* > + * > + * oFono - Open Source Telephony > + * > + * Copyright (C) 2010 Nokia Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > + */ > + > +#ifdef HAVE_CONFIG_H > +#include <config.h> > +#endif > + > +#define _GNU_SOURCE > +#include <string.h> > +#include <dirent.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <unistd.h> > + > +#include <glib.h> > + > +#include "cdma-smsutil.h" > + > +/* > + * Mapping from binary DTMF code to the digit it represents. > + * As defined in Table 2.7.1.3.2.4-4 of 3GPP2 C.S0005-E v2.0. > + * Note, 0 is NOT a valid value and not mapped to > + * any valid DTMF digit. > + */ > +const char cdma_sms_dtmf_digits[13] = {0, '1', '2', '3', '4', '5', '6', '7', > + '8', '9', '0', '*', '#'}; > + > + > +/* Unpacks the byte stream. */ > +static guint32 bit_field_unpack(const guint8 *buf, guint32 offset, > + guint32 nbit) > +{ > + guint32 val = 0; > + guint32 byteIndex; > + guint32 remainder; > + > + if (buf == NULL) > + return 0; > + > + byteIndex = offset >> 3; > + remainder = (offset & 0x7); > + > + if (remainder != 0) { > + /* > + * The field to be unpacked does not start at the byte > + * boundary. Retrieve those bits first. > + */ > + guint32 mask; > + > + mask = (1 << (8 - remainder)) - 1; > + val = buf[byteIndex] & mask; > + > + /* The field is within current byte */ > + if (nbit < (8 - remainder)) > + return (val >> (8 - remainder - nbit)); > + > + /* The field at least spans till end of the byte. */ > + nbit -= (8 - remainder); > + byteIndex++; > + } > + > + /* Unpack rest of the bits in the field in 8-bit chunk. */ > + while (nbit >= 8) { > + val = (val << 8) | buf[byteIndex]; > + nbit -= 8; > + byteIndex++; > + } > + > + /* If still some left, unpack the last remaining bits. */ > + if (nbit > 0) > + val = (val << nbit) | (buf[byteIndex] >> (8 - nbit)); > + > + return val; > +} So it looks to me like this function takes arbitrary bit fields from a stream. Is this a 'peculiarity' of the address field or will this be used elsewhere? This looks like a really inefficient function to call for every field, so it might makes things easier if the offsets were simply hardcoded... > + > +/* Convert CDMA DTMF digits into a string */ > +static gboolean cdma_sms_dtmf_to_ascii(char *buf, const char *addr, > + guint8 num_fields) > +{ > + guint8 index; > + guint8 value; > + > + if (buf == NULL || addr == NULL) > + return FALSE; > + > + for (index = 0; index < num_fields; index++) { > + if ((addr[index] <= 0) || (addr[index] > 12)) > + return FALSE; /* Invalid digit in address field */ > + > + value = (guint8) addr[index]; > + buf[index] = cdma_sms_dtmf_digits[value]; > + } > + > + buf[index] = 0; /* Make it NULL terminated string */ > + > + return TRUE; > +} > + > +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr) > +{ > + char *buf; > + > + if (addr == NULL) > + return NULL; > + > + if (addr->digit_mode != DTMF_4_BIT) > + return NULL; /* TODO: Only support DTMF_4_BIT currently */ > + > + buf = g_new(char, addr->num_fields + 1); > + if (buf == NULL) > + return NULL; > + > + if (!cdma_sms_dtmf_to_ascii(buf, addr->address, addr->num_fields)) { > + g_free(buf); > + return NULL; > + } > + > + return buf; > +} Does the address really need to be malloced? We have generally tried to avoid this in the past. The address usually goes directly to a dbus_message which takes care of copying the contents. Since oFono is not multi-threaded, using a static buffer might be sufficient here. > + > +/* Decode Address parameter record */ > +static gboolean cdma_sms_decode_addr(const unsigned char *buf, int len, > + struct cdma_sms_address *addr) > +{ > + guint32 bit_offset = 0; > + guint8 chari_len; > + guint32 total_num_bits = len * 8; > + guint8 index; > + > + if (len <= 0 || buf == NULL || addr == NULL) > + return FALSE; > + > + addr->digit_mode = (enum cdma_sms_digit_mode) Explicit casts should be avoided > + bit_field_unpack(buf, 0, 1); > + bit_offset += 1; Try to logically separate the fields by using an empty line between each one. > + addr->number_mode = (guint8) bit_field_unpack(buf, bit_offset, 1); > + bit_offset += 1; > + > + if (addr->digit_mode == CODE_8_BIT) { > + addr->number_type = (guint8) > + bit_field_unpack(buf, bit_offset, 3); > + bit_offset += 3; > + > + if (addr->number_mode == 0) { > + if (bit_offset + 4 > total_num_bits) > + return FALSE; > + > + addr->number_plan = (enum cdma_sms_numbering_plan) > + bit_field_unpack(buf, bit_offset, 4); > + bit_offset += 4; > + } > + } > + > + if ((bit_offset + 8) > total_num_bits) > + return FALSE; > + > + addr->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); > + bit_offset += 8; > + > + if (addr->digit_mode == DTMF_4_BIT) > + chari_len = 4; > + else > + chari_len = 8; > + > + if ((bit_offset + chari_len * addr->num_fields) > total_num_bits) > + return FALSE; > + > + for (index = 0; index < addr->num_fields; index++) { > + addr->address[index] = (char) bit_field_unpack(buf, > + bit_offset, > + chari_len); > + bit_offset += chari_len; > + } > + > + return TRUE; > +} > + > +char *cdma_sms_decode_text(struct cdma_sms_ud *ud) > +{ > + char *buf; > + > + if (ud == NULL) > + return NULL; > + > + /* TODO: Only support MSG_ENCODING_7BIT_ASCII currently */ > + if (ud->msg_encoding != MSG_ENCODING_7BIT_ASCII) > + return NULL; > + > + buf = g_new(char, ud->num_fields + 1); > + if (buf == NULL) > + return NULL; > + > + memcpy(buf, ud->chari, ud->num_fields); > + buf[ud->num_fields] = 0; /* Make it NULL terminated string */ > + > + return buf; > +} > + > +/* Decode User Data */ > +static gboolean cdma_sms_decode_ud(const unsigned char *buf, int len, > + struct cdma_sms_ud *ud) > +{ > + guint32 bit_offset = 0; > + guint8 chari_len = 0; > + guint32 total_num_bits = len * 8; > + guint8 index; > + enum cdma_sms_message_encoding msg_encoding; > + > + if (buf == NULL || ud == NULL) > + return FALSE; > + > + if (total_num_bits < 13) > + return FALSE; > + > + msg_encoding = (enum cdma_sms_message_encoding) > + bit_field_unpack(buf, bit_offset, 5); > + ud->msg_encoding = msg_encoding; > + bit_offset += 5; > + > + if (ud->msg_encoding == MSG_ENCODING_EXTENDED_PROTOCOL_MSG || > + ud->msg_encoding == MSG_ENCODING_GSM_DATA_CODING) { > + /* Skip message type field */ > + /* TODO: Add support for message type field */ > + bit_offset += 8; > + } > + > + if (bit_offset + 8 > total_num_bits) > + return FALSE; > + > + ud->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); > + bit_offset += 8; > + > + switch (msg_encoding) { > + case MSG_ENCODING_OCTET: > + chari_len = 8; > + break; > + case MSG_ENCODING_EXTENDED_PROTOCOL_MSG: > + return FALSE; /* TODO */ > + case MSG_ENCODING_7BIT_ASCII: > + case MSG_ENCODING_IA5: > + chari_len = 7; > + break; > + case MSG_ENCODING_UNICODE: > + case MSG_ENCODING_SHIFT_JIS: > + case MSG_ENCODING_KOREAN: > + return FALSE; /* TODO */ > + case MSG_ENCODING_LATIN_HEBREW: > + case MSG_ENCODING_LATIN: > + chari_len = 8; > + break; > + case MSG_ENCODING_GSM_7BIT: > + chari_len = 7; > + break; > + case MSG_ENCODING_GSM_DATA_CODING: > + return FALSE; /* TODO */ > + } > + > + if (chari_len == 0) > + return FALSE; > + > + if (bit_offset + chari_len * ud->num_fields > total_num_bits) > + return FALSE; > + > + for (index = 0; index < ud->num_fields; index++) { > + ud->chari[index] = (guint8) bit_field_unpack(buf, > + bit_offset, > + chari_len); > + bit_offset += chari_len; > + } > + > + return TRUE; > +} > + > +/* Decode Message Identifier */ > +static gboolean cdma_sms_decode_message_id(const unsigned char *buf, int len, > + struct cdma_sms_identifier *id) > +{ > + guint32 bit_offset = 0; > + > + if (buf == NULL || id == NULL) > + return FALSE; > + > + if (len != 3) > + return FALSE; > + > + id->msg_type = (enum cdma_sms_msg_type) > + bit_field_unpack(buf, bit_offset, 4); > + bit_offset += 4; > + id->msg_id = (guint16) bit_field_unpack(buf, bit_offset, 16); > + bit_offset += 16; > + id->header_ind = (gboolean) bit_field_unpack(buf, bit_offset, 1); > + > + return TRUE; > +} > + > +/* Decode Bearer Data */ > +static gboolean cdma_sms_decode_bearer_data(const unsigned char *buf, int len, > + struct cdma_sms_bearer_data *bearer_data) > +{ > + gboolean ret = TRUE; > + enum cdma_sms_subparam_id subparam_id; > + guint8 subparam_len; > + > + if (buf == NULL || bearer_data == NULL) > + return FALSE; > + Have you looked at how STK pdus are decoded in stkutil.c, parse_dataobj() in particular. Looking at the basic code structure so far, that design pattern could be (or not) a really nice fit here and save you some kLoC in the future. > + while (len != 0) { > + > + if (len < 2) > + return FALSE; > + > + subparam_id = (enum cdma_sms_subparam_id) > + bit_field_unpack(buf, 0, 8); > + buf += 1; > + subparam_len = (guint8) bit_field_unpack(buf, 0, 8); > + buf += 1; > + > + len -= 2; > + > + if (len < subparam_len) > + return FALSE; > + > + switch (subparam_id) { > + case CDMA_SMS_SUBPARAM_ID_MESSAGE_ID: > + ret = cdma_sms_decode_message_id(buf, subparam_len, > + &bearer_data->id); > + set_bitmap(&bearer_data->subparam_bitmap, > + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID); > + break; > + case CDMA_SMS_SUBPARAM_ID_USER_DATA: > + ret = cdma_sms_decode_ud(buf, subparam_len, > + &bearer_data->ud); > + set_bitmap(&bearer_data->subparam_bitmap, > + CDMA_SMS_SUBPARAM_ID_USER_DATA); > + break; > + case CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE: > + case CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP: > + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE: > + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE: > + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE: > + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE: > + case CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR: > + case CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR: > + case CDMA_SMS_SUBPARAM_ID_REPLY_OPTION: > + case CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES: > + case CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY: > + case CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR: > + case CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER: > + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE: > + case CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA: > + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX: > + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA: > + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT: > + case CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS: > + case CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE: > + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN: > + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK: > + /* > + * TODO: Ignore any unsupported parameter. > + * Also ignore any un-recognized parameter for > + * future standard revision. > + */ > + break; > + } > + > + if (ret == FALSE) > + return FALSE; > + > + len -= subparam_len; > + buf += subparam_len; > + } > + > + return TRUE; > +} > + > +static gboolean cdma_sms_p2p_decode(const unsigned char *pdu, int len, > + struct cdma_sms *incoming) > +{ > + gboolean ret = TRUE; > + enum cdma_sms_param_id rec_id; > + guint8 rec_len; > + > + if (pdu == NULL || incoming == NULL) > + return FALSE; > + > + while (len != 0) { > + > + if (len <= 2) > + return FALSE; > + > + rec_id = (enum cdma_sms_param_id) bit_field_unpack(pdu, 0, 8); > + pdu += 1; > + rec_len = (guint8) bit_field_unpack(pdu, 0, 8); > + pdu += 1; > + > + len -= 2; > + > + if (len < rec_len) > + return FALSE; > + > + switch (rec_id) { > + case CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER: > + incoming->p2p_msg.teleservice_id = > + (enum cdma_sms_teleservice_id) > + bit_field_unpack(pdu, 0, 16); > + set_bitmap(&incoming->p2p_msg.param_bitmap, > + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER); > + break; > + case CDMA_SMS_PARAM_ID_SERVICE_CATEGORY: > + break; /* TODO */ > + case CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS: > + ret = cdma_sms_decode_addr(pdu, rec_len, > + &incoming->p2p_msg.oaddr); > + set_bitmap(&incoming->p2p_msg.param_bitmap, > + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS); > + break; > + case CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS: > + case CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS: > + case CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS: > + case CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION: > + case CDMA_SMS_PARAM_ID_CAUSE_CODE: > + break; /* TODO */ > + case CDMA_SMS_PARAM_ID_BEARER_DATA: > + ret = cdma_sms_decode_bearer_data(pdu, rec_len, > + &incoming->p2p_msg.bearer_data); > + set_bitmap(&incoming->p2p_msg.param_bitmap, > + CDMA_SMS_PARAM_ID_BEARER_DATA); > + break; > + } > + > + if (!ret) > + return FALSE; > + > + len -= rec_len; > + pdu += rec_len; > + } > + > + return TRUE; > +} > + > +gboolean cdma_sms_decode(const unsigned char *pdu, int len, > + struct cdma_sms *incoming) > +{ > + gboolean ret = FALSE; The preference is not to initialize this variable. If you can't find a nicer way around it, and if the compiler complains, then use the uninitialized_var() trick from smsutil.c. > + > + if ((len == 0) || (pdu == NULL) || (incoming == NULL)) > + return FALSE; > + > + incoming->type = (enum cdma_sms_tp_msg_type) > + bit_field_unpack(pdu, 0, 8); > + pdu += 1; > + len -= 1; > + > + switch (incoming->type) { > + case CDMA_SMS_P2P: > + ret = cdma_sms_p2p_decode(pdu, len, incoming); I suggest a simple return cdma_sms_p2p_decode() here.. > + break; > + case CDMA_SMS_BCAST: > + case CDMA_SMS_ACK: > + /* TODO: Not supported yet */ > + ret = FALSE; > + break; and a return FALSE here > + } > + > + return ret; and a return FALSE here. > +} > diff --git a/src/cdma-smsutil.h b/src/cdma-smsutil.h > new file mode 100644 > index 0000000..bfe2abb > --- /dev/null > +++ b/src/cdma-smsutil.h > @@ -0,0 +1,264 @@ > +/* > + * > + * oFono - Open Source Telephony > + * > + * Copyright (C) 2010 Nokia Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > + */ > + > +/* 3GPP2 C.S0015-B v2.0, Table 3.4-1 */ > +enum cdma_sms_tp_msg_type { > + CDMA_SMS_P2P = 0, > + CDMA_SMS_BCAST = 1, > + CDMA_SMS_ACK = 2 > +}; > + > +/* 3GPP2 X.S0004-550-E, Section 2.256 */ > +enum cdma_sms_teleservice_id { > + TELESERVICE_CMT91 = 4096, > + TELESERVICE_WPT = 4097, > + TELESERVICE_WMT = 4098, > + TELESERVICE_VMN = 4099, > + TELESERVICE_WAP = 4100, > + TELESERVICE_WEMT = 4101, > + TELESERVICE_SCPT = 4102, > + TELESERVICE_CATPT = 4103 > +}; > + > +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ > +enum cdma_sms_digi_number_type { > + CDMA_SMS_NUM_TYPE_UNKNOWN = 0, > + CDMA_SMS_NUM_TYPE_INTERNATIONAL_NUMBER = 1, > + CDMA_SMS_NUM_TYPE_NATIONAL_NUMBER = 2, > + CDMA_SMS_NUM_TYPE_NETWORK_SPECIFIC_NUMBER = 3, > + CDMA_SMS_NUM_TYPE_SUBSCRIBER_NUMBER = 4, > + /* Reserved 5 */ > + CDMA_SMS_NUM_TYPE_ABBREVIATED_NUMBER = 6 > + /* Reserved 7 */ > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ > +enum cdma_sms_data_network_number_type { > + CDMA_SMS_NETWORK_NUM_TYPE_UNKNOWN = 0, > + CDMA_SMS_NETWORK_TYPE_INTERNET_PROTOCOL = 1, > + CDMA_SMS_NETWORK_TYPE_INTERNET_EMAIL_ADDRESS = 2, > + /* All Other Values Reserved */ > +}; > + > +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-3 */ > +enum cdma_sms_numbering_plan { > + CDMA_SMS_NUMBERING_PLAN_UNKNOWN = 0, > + CDMA_SMS_NUMBERING_PLAN_ISDN = 1, > + CDMA_SMS_NUMBERING_PLAN_DATA = 3, > + CDMA_SMS_NUMBERING_PLAN_TELEX = 4, > + CDMA_SMS_NUMBERING_PLAN_PRIVATE = 9, > + CDMA_SMS_NUMBERING_PLAN_RESERVED = 15 > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ > +enum cdma_sms_msg_type { > + CDMA_SMS_RESERVED = 0, > + CDMA_SMS_DELIVER = 1, > + CDMA_SMS_SUBMIT = 2, > + CDMA_SMS_CANCEL = 3, > + CDMA_SMS_DELIVER_ACK = 4, > + CDMA_SMS_USER_ACK = 5, > + CDMA_SMS_READ_ACK = 6, > +}; > + > +/* C.R1001-G_v1.0 Table 9.1-1 */ > +enum cdma_sms_message_encoding { > + MSG_ENCODING_OCTET = 0, > + MSG_ENCODING_EXTENDED_PROTOCOL_MSG = 1, > + MSG_ENCODING_7BIT_ASCII = 2, > + MSG_ENCODING_IA5 = 3, > + MSG_ENCODING_UNICODE = 4, > + MSG_ENCODING_SHIFT_JIS = 5, > + MSG_ENCODING_KOREAN = 6, > + MSG_ENCODING_LATIN_HEBREW = 7, > + MSG_ENCODING_LATIN = 8, > + MSG_ENCODING_GSM_7BIT = 9, > + MSG_ENCODING_GSM_DATA_CODING = 10 > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3-1 */ > +enum cdma_sms_param_id { > + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER = 0x00, > + CDMA_SMS_PARAM_ID_SERVICE_CATEGORY = 0x01, > + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS = 0x02, > + CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS = 0x03, > + CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS = 0x04, > + CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS = 0x05, > + CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION = 0x06, > + CDMA_SMS_PARAM_ID_CAUSE_CODE = 0x07, > + CDMA_SMS_PARAM_ID_BEARER_DATA = 0x08 > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 4.5-1 */ > +enum cdma_sms_subparam_id { > + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID = 0x00, > + CDMA_SMS_SUBPARAM_ID_USER_DATA = 0x01, > + CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE = 0x02, > + CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP = 0x03, > + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE = 0x04, > + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE = 0x05, > + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE = 0x06, > + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE = 0x07, > + CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR = 0x08, > + CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR = 0x09, > + CDMA_SMS_SUBPARAM_ID_REPLY_OPTION = 0x0A, > + CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES = 0x0B, > + CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY = 0x0C, > + CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR = 0x0D, > + CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER = 0x0E, > + CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE = 0x0F, > + CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA = 0x10, > + CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX = 0x11, > + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA = 0x12, > + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT = 0x13, > + CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS = 0x14, > + CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE = 0x15, > + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN = 0x16, > + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK = 0x17 > +}; > + > +/* 3GPP2 C.R1001-G Table 9.3.1-1 */ > +enum cdma_sms_service_category { > + SERVICE_CAT_EMERGENCY_BROADCAST = 1, > + SERVICE_CAT_ADMINISTRATIVE = 2, > + SERVICE_CAT_MAINTENANCE = 3, > + SERVICE_CAT_GENERALNEWSLOCAL = 4, Please use the underscore some more, e.g. GENERAL_NEWS_LOCAL > + SERVICE_CAT_GENERALNEWSREGIONAL = 5, > + SERVICE_CAT_GENERALNEWSNATIONAL = 6, > + SERVICE_CAT_GENERALNEWSINTERNATIONAL = 7, > + SERVICE_CAT_BUSINESSFINALNEWSLOCAL = 8, > + SERVICE_CAT_BUSINESSFINALNEWSREGIONAL = 9, > + SERVICE_CAT_BUSINESSFINALNEWSNATIONAL = 10, > + SERVICE_CAT_BUSINESSFINALNEWSINTNL = 11, > + SERVICE_CAT_SPORTSNEWSLOCAL = 12, > + SERVICE_CAT_SPORTSNEWSREGIONAL = 13, > + SERVICE_CAT_SPORTSNEWSNATIONAL = 14, > + SERVICE_CAT_SPORTSNEWSINTERNATIONAL = 15, > + SERVICE_CAT_ENTERTAINMENTNEWSLOCAL = 16, > + SERVICE_CAT_ENTERTAINMENTNEWSREGIONAL = 17, > + SERVICE_CAT_ENTERTAINMENTNEWSNATIONAL = 18, > + SERVICE_CAT_ENTERTAINMENTNEWSINTERNATIONAL = 19, > + SERVICE_CAT_ENTERTAINMENTNEWSLOCALWEATHER = 20, > + SERVICE_CAT_AREATRAFFICREPORTS = 21, > + SERVICE_CAT_LOCALAIRTPORTFLIGHTSCHEDULES = 22, > + SERVICE_CAT_RESTURANTS = 23, Is this mis-spelled? > + SERVICE_CAT_LODGINGS = 24, > + SERVICE_CAT_RETAILDIRECTORYADVERTISEMENTS = 25, > + SERVICE_CAT_ADVERTISEMENTS = 26, > + SERVICE_CAT_STOCKQUOTES = 27, > + SERVICE_CAT_EMPLOYMENTOPPORTUNITIES = 28, > + SERVICE_CAT_MEDICALHEALTHHOSPITALS = 29, > + SERVICE_CAT_TECHNOLOGYNEWS = 30, > + SERVICE_CAT_MULTICATEGORY = 31, > + SERVICE_CAT_CAPT = 32 > +}; > + > +enum cdma_sms_digit_mode { > + DTMF_4_BIT = 0, > + CODE_8_BIT = 1 > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ > +struct cdma_sms_address { > + enum cdma_sms_digit_mode digit_mode; > + guint8 number_mode; > + guint8 number_type; > + enum cdma_sms_numbering_plan number_plan; > + guint8 num_fields; > + char address[256]; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.6 */ > +struct cdma_sms_cause_code { > + guint8 reply_seq; > + guint8 error_class; > + guint8 cause_code; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 4.5.1 */ > +struct cdma_sms_identifier { > + enum cdma_sms_msg_type msg_type; > + guint16 msg_id; > + gboolean header_ind; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 4.5.2 */ > +struct cdma_sms_ud { > + enum cdma_sms_message_encoding msg_encoding; > + guint8 num_fields; > + guint8 chari[512]; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 4.5 */ > +struct cdma_sms_bearer_data { > + guint32 subparam_bitmap; > + struct cdma_sms_identifier id; > + struct cdma_sms_ud ud; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.1-1. */ > +struct cdma_sms_p2p_msg { > + guint32 param_bitmap; > + enum cdma_sms_teleservice_id teleservice_id; > + struct cdma_sms_address oaddr; > + struct cdma_sms_bearer_data bearer_data; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.2-1 */ > +struct cdma_sms_broadcast_msg { > + enum cdma_sms_service_category service_category; > + struct cdma_sms_bearer_data bearer_data; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.3-1 */ > +struct cdma_sms_ack_msg { > + struct cdma_sms_address daddr; > + struct cdma_sms_cause_code cause_code; > +}; > + > +/* 3GPP2 C.S0015-B v2.0 Section 3.4.1 */ > +struct cdma_sms { > + enum cdma_sms_tp_msg_type type; > + union { > + struct cdma_sms_p2p_msg p2p_msg; > + struct cdma_sms_broadcast_msg broadcast_msg; > + struct cdma_sms_ack_msg ack_msg; > + }; > +}; > + > +static inline gboolean check_bitmap(guint32 bitmap, guint32 pos) > +{ > + guint32 mask = 0x1 << pos; > + return bitmap & mask ? TRUE : FALSE; > +} > + I don't see this function being used by this patch. Does it logically belong to a later patch? > +static inline void set_bitmap(guint32 *bitmap, guint32 pos) > +{ > + if (bitmap == NULL) > + return; > + > + *bitmap = *bitmap | (1 << pos); > +} Why are these functions defined in the header? Are they meant to be used elsewhere? > + > +gboolean cdma_sms_decode(const unsigned char *pdu, int len, > + struct cdma_sms *out); > +char *cdma_sms_decode_text(struct cdma_sms_ud *ud); > +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr); Regards, -Denis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support 2011-01-04 21:17 ` Denis Kenzior @ 2011-01-05 17:47 ` Lei Yu 2011-01-06 20:23 ` Denis Kenzior 0 siblings, 1 reply; 6+ messages in thread From: Lei Yu @ 2011-01-05 17:47 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 35181 bytes --] Hi Denis, On 01/04/2011 01:17 PM, ext Denis Kenzior wrote: > Hi Lei, > > On 12/21/2010 06:02 PM, Lei Yu wrote: >> --- >> Makefile.am | 3 +- >> src/cdma-smsutil.c | 484 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> src/cdma-smsutil.h | 264 ++++++++++++++++++++++++++++ >> 3 files changed, 750 insertions(+), 1 deletions(-) >> create mode 100644 src/cdma-smsutil.c >> create mode 100644 src/cdma-smsutil.h >> >> diff --git a/Makefile.am b/Makefile.am >> index aa00016..e85f522 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -319,7 +319,8 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \ >> src/radio-settings.c src/stkutil.h src/stkutil.c \ >> src/nettime.c src/stkagent.c src/stkagent.h \ >> src/simfs.c src/simfs.h src/audio-settings.c \ >> - src/smsagent.c src/smsagent.h src/ctm.c >> + src/smsagent.c src/smsagent.h src/ctm.c \ >> + src/cdma-smsutil.c > > Make sure to include src/cdma-smsutil.h as well. > > I'm still not really familiar with the CDMA SMS pdu spec, so take my > comments with a grain of salt ;) > Will fix. >> >> src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl >> >> diff --git a/src/cdma-smsutil.c b/src/cdma-smsutil.c >> new file mode 100644 >> index 0000000..b80002c >> --- /dev/null >> +++ b/src/cdma-smsutil.c >> @@ -0,0 +1,484 @@ >> +/* >> + * >> + * oFono - Open Source Telephony >> + * >> + * Copyright (C) 2010 Nokia Corporation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >> + * >> + */ >> + >> +#ifdef HAVE_CONFIG_H >> +#include<config.h> >> +#endif >> + >> +#define _GNU_SOURCE >> +#include<string.h> >> +#include<dirent.h> >> +#include<sys/types.h> >> +#include<sys/stat.h> >> +#include<unistd.h> >> + >> +#include<glib.h> >> + >> +#include "cdma-smsutil.h" >> + >> +/* >> + * Mapping from binary DTMF code to the digit it represents. >> + * As defined in Table 2.7.1.3.2.4-4 of 3GPP2 C.S0005-E v2.0. >> + * Note, 0 is NOT a valid value and not mapped to >> + * any valid DTMF digit. >> + */ >> +const char cdma_sms_dtmf_digits[13] = {0, '1', '2', '3', '4', '5', '6', '7', >> + '8', '9', '0', '*', '#'}; >> + >> + >> +/* Unpacks the byte stream. */ >> +static guint32 bit_field_unpack(const guint8 *buf, guint32 offset, >> + guint32 nbit) >> +{ >> + guint32 val = 0; >> + guint32 byteIndex; >> + guint32 remainder; >> + >> + if (buf == NULL) >> + return 0; >> + >> + byteIndex = offset>> 3; >> + remainder = (offset& 0x7); >> + >> + if (remainder != 0) { >> + /* >> + * The field to be unpacked does not start at the byte >> + * boundary. Retrieve those bits first. >> + */ >> + guint32 mask; >> + >> + mask = (1<< (8 - remainder)) - 1; >> + val = buf[byteIndex]& mask; >> + >> + /* The field is within current byte */ >> + if (nbit< (8 - remainder)) >> + return (val>> (8 - remainder - nbit)); >> + >> + /* The field at least spans till end of the byte. */ >> + nbit -= (8 - remainder); >> + byteIndex++; >> + } >> + >> + /* Unpack rest of the bits in the field in 8-bit chunk. */ >> + while (nbit>= 8) { >> + val = (val<< 8) | buf[byteIndex]; >> + nbit -= 8; >> + byteIndex++; >> + } >> + >> + /* If still some left, unpack the last remaining bits. */ >> + if (nbit> 0) >> + val = (val<< nbit) | (buf[byteIndex]>> (8 - nbit)); >> + >> + return val; >> +} > > So it looks to me like this function takes arbitrary bit fields from a > stream. Is this a 'peculiarity' of the address field or will this be > used elsewhere? > Yes, your interpretation of the function is correct. Actually, cross CDMA SMS spec (and also other CDMA protocol specs), a lot of the message (parameter and etc) structures are defined just as 'peculiar' as Address field. For example: User Data (section 4.5.2 of C.S0015-B), Call Back Number (can be considered same as address). Essentially, CDMA SMS specs defines structures as bit streams and a field can start at any offset within the bit stream. Even worse, depending on the values of the previous fields, a field can start at different locations within a bit stream for the different instances of the same type of the message. > This looks like a really inefficient function to call for every field, > so it might makes things easier if the offsets were simply hardcoded... > The main reason I do this way is that I feel there will be more chances to make mistake if the 'shift', 'bit-wise and', bit-wise or' operation is carried out when decoding each field separately. It will be cleaner to have this generic function and decoding of each field will just care about size of the field and start offset of the field into the stream. Efficiency-wise, I see worst case a few extra machine instructions will be used in comparing to 'manually' decoding at each place. For harcode the offset, I guess you mean replacing following code segment bit_offset +=x next_field = bit_field_unpack(buf, bit_offset, next_field_len); with something like next_field = bit_field_unpack(buf, N, next_field_len); If this this what you mean, yes, N can be hard-coded in many places but still quite a few places bit_offset has to be calculated due the reasons explained before. But, again, efficiency-wise, this will only save one add instruction. If you still see a need, I can sure fix those. >> + >> +/* Convert CDMA DTMF digits into a string */ >> +static gboolean cdma_sms_dtmf_to_ascii(char *buf, const char *addr, >> + guint8 num_fields) >> +{ >> + guint8 index; >> + guint8 value; >> + >> + if (buf == NULL || addr == NULL) >> + return FALSE; >> + >> + for (index = 0; index< num_fields; index++) { >> + if ((addr[index]<= 0) || (addr[index]> 12)) >> + return FALSE; /* Invalid digit in address field */ >> + >> + value = (guint8) addr[index]; >> + buf[index] = cdma_sms_dtmf_digits[value]; >> + } >> + >> + buf[index] = 0; /* Make it NULL terminated string */ >> + >> + return TRUE; >> +} >> + >> +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr) >> +{ >> + char *buf; >> + >> + if (addr == NULL) >> + return NULL; >> + >> + if (addr->digit_mode != DTMF_4_BIT) >> + return NULL; /* TODO: Only support DTMF_4_BIT currently */ >> + >> + buf = g_new(char, addr->num_fields + 1); >> + if (buf == NULL) >> + return NULL; >> + >> + if (!cdma_sms_dtmf_to_ascii(buf, addr->address, addr->num_fields)) { >> + g_free(buf); >> + return NULL; >> + } >> + >> + return buf; >> +} > > Does the address really need to be malloced? We have generally tried to > avoid this in the past. The address usually goes directly to a > dbus_message which takes care of copying the contents. Since oFono is > not multi-threaded, using a static buffer might be sufficient here. > Agree. Will fix. >> + >> +/* Decode Address parameter record */ >> +static gboolean cdma_sms_decode_addr(const unsigned char *buf, int len, >> + struct cdma_sms_address *addr) >> +{ >> + guint32 bit_offset = 0; >> + guint8 chari_len; >> + guint32 total_num_bits = len * 8; >> + guint8 index; >> + >> + if (len<= 0 || buf == NULL || addr == NULL) >> + return FALSE; >> + >> + addr->digit_mode = (enum cdma_sms_digit_mode) > > Explicit casts should be avoided Will fix. Could you pls also educate me the rationales behind this rule? > >> + bit_field_unpack(buf, 0, 1); >> + bit_offset += 1; > > Try to logically separate the fields by using an empty line between each > one. > Will fix cross the file. >> + addr->number_mode = (guint8) bit_field_unpack(buf, bit_offset, 1); >> + bit_offset += 1; >> + >> + if (addr->digit_mode == CODE_8_BIT) { >> + addr->number_type = (guint8) >> + bit_field_unpack(buf, bit_offset, 3); >> + bit_offset += 3; >> + >> + if (addr->number_mode == 0) { >> + if (bit_offset + 4> total_num_bits) >> + return FALSE; >> + >> + addr->number_plan = (enum cdma_sms_numbering_plan) >> + bit_field_unpack(buf, bit_offset, 4); >> + bit_offset += 4; >> + } >> + } >> + >> + if ((bit_offset + 8)> total_num_bits) >> + return FALSE; >> + >> + addr->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); >> + bit_offset += 8; >> + >> + if (addr->digit_mode == DTMF_4_BIT) >> + chari_len = 4; >> + else >> + chari_len = 8; >> + >> + if ((bit_offset + chari_len * addr->num_fields)> total_num_bits) >> + return FALSE; >> + >> + for (index = 0; index< addr->num_fields; index++) { >> + addr->address[index] = (char) bit_field_unpack(buf, >> + bit_offset, >> + chari_len); >> + bit_offset += chari_len; >> + } >> + >> + return TRUE; >> +} >> + >> +char *cdma_sms_decode_text(struct cdma_sms_ud *ud) >> +{ >> + char *buf; >> + >> + if (ud == NULL) >> + return NULL; >> + >> + /* TODO: Only support MSG_ENCODING_7BIT_ASCII currently */ >> + if (ud->msg_encoding != MSG_ENCODING_7BIT_ASCII) >> + return NULL; >> + >> + buf = g_new(char, ud->num_fields + 1); >> + if (buf == NULL) >> + return NULL; >> + >> + memcpy(buf, ud->chari, ud->num_fields); >> + buf[ud->num_fields] = 0; /* Make it NULL terminated string */ >> + >> + return buf; >> +} >> + >> +/* Decode User Data */ >> +static gboolean cdma_sms_decode_ud(const unsigned char *buf, int len, >> + struct cdma_sms_ud *ud) >> +{ >> + guint32 bit_offset = 0; >> + guint8 chari_len = 0; >> + guint32 total_num_bits = len * 8; >> + guint8 index; >> + enum cdma_sms_message_encoding msg_encoding; >> + >> + if (buf == NULL || ud == NULL) >> + return FALSE; >> + >> + if (total_num_bits< 13) >> + return FALSE; >> + >> + msg_encoding = (enum cdma_sms_message_encoding) >> + bit_field_unpack(buf, bit_offset, 5); >> + ud->msg_encoding = msg_encoding; >> + bit_offset += 5; >> + >> + if (ud->msg_encoding == MSG_ENCODING_EXTENDED_PROTOCOL_MSG || >> + ud->msg_encoding == MSG_ENCODING_GSM_DATA_CODING) { >> + /* Skip message type field */ >> + /* TODO: Add support for message type field */ >> + bit_offset += 8; >> + } >> + >> + if (bit_offset + 8> total_num_bits) >> + return FALSE; >> + >> + ud->num_fields = (guint8) bit_field_unpack(buf, bit_offset, 8); >> + bit_offset += 8; >> + >> + switch (msg_encoding) { >> + case MSG_ENCODING_OCTET: >> + chari_len = 8; >> + break; >> + case MSG_ENCODING_EXTENDED_PROTOCOL_MSG: >> + return FALSE; /* TODO */ >> + case MSG_ENCODING_7BIT_ASCII: >> + case MSG_ENCODING_IA5: >> + chari_len = 7; >> + break; >> + case MSG_ENCODING_UNICODE: >> + case MSG_ENCODING_SHIFT_JIS: >> + case MSG_ENCODING_KOREAN: >> + return FALSE; /* TODO */ >> + case MSG_ENCODING_LATIN_HEBREW: >> + case MSG_ENCODING_LATIN: >> + chari_len = 8; >> + break; >> + case MSG_ENCODING_GSM_7BIT: >> + chari_len = 7; >> + break; >> + case MSG_ENCODING_GSM_DATA_CODING: >> + return FALSE; /* TODO */ >> + } >> + >> + if (chari_len == 0) >> + return FALSE; >> + >> + if (bit_offset + chari_len * ud->num_fields> total_num_bits) >> + return FALSE; >> + >> + for (index = 0; index< ud->num_fields; index++) { >> + ud->chari[index] = (guint8) bit_field_unpack(buf, >> + bit_offset, >> + chari_len); >> + bit_offset += chari_len; >> + } >> + >> + return TRUE; >> +} >> + >> +/* Decode Message Identifier */ >> +static gboolean cdma_sms_decode_message_id(const unsigned char *buf, int len, >> + struct cdma_sms_identifier *id) >> +{ >> + guint32 bit_offset = 0; >> + >> + if (buf == NULL || id == NULL) >> + return FALSE; >> + >> + if (len != 3) >> + return FALSE; >> + >> + id->msg_type = (enum cdma_sms_msg_type) >> + bit_field_unpack(buf, bit_offset, 4); >> + bit_offset += 4; >> + id->msg_id = (guint16) bit_field_unpack(buf, bit_offset, 16); >> + bit_offset += 16; >> + id->header_ind = (gboolean) bit_field_unpack(buf, bit_offset, 1); >> + >> + return TRUE; >> +} >> + >> +/* Decode Bearer Data */ >> +static gboolean cdma_sms_decode_bearer_data(const unsigned char *buf, int len, >> + struct cdma_sms_bearer_data *bearer_data) >> +{ >> + gboolean ret = TRUE; >> + enum cdma_sms_subparam_id subparam_id; >> + guint8 subparam_len; >> + >> + if (buf == NULL || bearer_data == NULL) >> + return FALSE; >> + > > Have you looked at how STK pdus are decoded in stkutil.c, > parse_dataobj() in particular. Looking at the basic code structure so > far, that design pattern could be (or not) a really nice fit here and > save you some kLoC in the future. > Looked at it. I am assuming that the design pattern you are referring to is that we can create an array of structure where decoding function for a parameter record can be stored thus, this part of the code will be reduced to something like: parsing the type and invoke the decoding function (dataobj_handler as in stkutil.c) as in the array of the function handlers. I don't see this approach and the approach I described above are really not much differences since the logics below are nothing but a big switch-case state which will be required even with function handler array approach anyway. The only simplification I can see below is to somehow to put set_bitmap() into one place. This will reduce ~30 LOC. >> + while (len != 0) { >> + >> + if (len< 2) >> + return FALSE; >> + >> + subparam_id = (enum cdma_sms_subparam_id) >> + bit_field_unpack(buf, 0, 8); >> + buf += 1; >> + subparam_len = (guint8) bit_field_unpack(buf, 0, 8); >> + buf += 1; >> + >> + len -= 2; >> + >> + if (len< subparam_len) >> + return FALSE; >> + >> + switch (subparam_id) { >> + case CDMA_SMS_SUBPARAM_ID_MESSAGE_ID: >> + ret = cdma_sms_decode_message_id(buf, subparam_len, >> +&bearer_data->id); >> + set_bitmap(&bearer_data->subparam_bitmap, >> + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID); >> + break; >> + case CDMA_SMS_SUBPARAM_ID_USER_DATA: >> + ret = cdma_sms_decode_ud(buf, subparam_len, >> +&bearer_data->ud); >> + set_bitmap(&bearer_data->subparam_bitmap, >> + CDMA_SMS_SUBPARAM_ID_USER_DATA); >> + break; >> + case CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE: >> + case CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP: >> + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE: >> + case CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE: >> + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE: >> + case CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE: >> + case CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR: >> + case CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR: >> + case CDMA_SMS_SUBPARAM_ID_REPLY_OPTION: >> + case CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES: >> + case CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY: >> + case CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR: >> + case CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER: >> + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE: >> + case CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA: >> + case CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX: >> + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA: >> + case CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT: >> + case CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS: >> + case CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE: >> + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN: >> + case CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK: >> + /* >> + * TODO: Ignore any unsupported parameter. >> + * Also ignore any un-recognized parameter for >> + * future standard revision. >> + */ >> + break; >> + } >> + >> + if (ret == FALSE) >> + return FALSE; >> + >> + len -= subparam_len; >> + buf += subparam_len; >> + } >> + >> + return TRUE; >> +} >> + >> +static gboolean cdma_sms_p2p_decode(const unsigned char *pdu, int len, >> + struct cdma_sms *incoming) >> +{ >> + gboolean ret = TRUE; >> + enum cdma_sms_param_id rec_id; >> + guint8 rec_len; >> + >> + if (pdu == NULL || incoming == NULL) >> + return FALSE; >> + >> + while (len != 0) { >> + >> + if (len<= 2) >> + return FALSE; >> + >> + rec_id = (enum cdma_sms_param_id) bit_field_unpack(pdu, 0, 8); >> + pdu += 1; >> + rec_len = (guint8) bit_field_unpack(pdu, 0, 8); >> + pdu += 1; >> + >> + len -= 2; >> + >> + if (len< rec_len) >> + return FALSE; >> + >> + switch (rec_id) { >> + case CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER: >> + incoming->p2p_msg.teleservice_id = >> + (enum cdma_sms_teleservice_id) >> + bit_field_unpack(pdu, 0, 16); >> + set_bitmap(&incoming->p2p_msg.param_bitmap, >> + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER); >> + break; >> + case CDMA_SMS_PARAM_ID_SERVICE_CATEGORY: >> + break; /* TODO */ >> + case CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS: >> + ret = cdma_sms_decode_addr(pdu, rec_len, >> +&incoming->p2p_msg.oaddr); >> + set_bitmap(&incoming->p2p_msg.param_bitmap, >> + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS); >> + break; >> + case CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS: >> + case CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS: >> + case CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS: >> + case CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION: >> + case CDMA_SMS_PARAM_ID_CAUSE_CODE: >> + break; /* TODO */ >> + case CDMA_SMS_PARAM_ID_BEARER_DATA: >> + ret = cdma_sms_decode_bearer_data(pdu, rec_len, >> +&incoming->p2p_msg.bearer_data); >> + set_bitmap(&incoming->p2p_msg.param_bitmap, >> + CDMA_SMS_PARAM_ID_BEARER_DATA); >> + break; >> + } >> + >> + if (!ret) >> + return FALSE; >> + >> + len -= rec_len; >> + pdu += rec_len; >> + } >> + >> + return TRUE; >> +} >> + >> +gboolean cdma_sms_decode(const unsigned char *pdu, int len, >> + struct cdma_sms *incoming) >> +{ >> + gboolean ret = FALSE; > > The preference is not to initialize this variable. If you can't find a > nicer way around it, and if the compiler complains, then use the > uninitialized_var() trick from smsutil.c. > Will fix. >> + >> + if ((len == 0) || (pdu == NULL) || (incoming == NULL)) >> + return FALSE; >> + >> + incoming->type = (enum cdma_sms_tp_msg_type) >> + bit_field_unpack(pdu, 0, 8); >> + pdu += 1; >> + len -= 1; >> + >> + switch (incoming->type) { >> + case CDMA_SMS_P2P: >> + ret = cdma_sms_p2p_decode(pdu, len, incoming); > > I suggest a simple return cdma_sms_p2p_decode() here.. > Will fix. >> + break; >> + case CDMA_SMS_BCAST: >> + case CDMA_SMS_ACK: >> + /* TODO: Not supported yet */ >> + ret = FALSE; >> + break; > > and a return FALSE here > Will fix. >> + } >> + >> + return ret; > > and a return FALSE here. > Will fix. >> +} >> diff --git a/src/cdma-smsutil.h b/src/cdma-smsutil.h >> new file mode 100644 >> index 0000000..bfe2abb >> --- /dev/null >> +++ b/src/cdma-smsutil.h >> @@ -0,0 +1,264 @@ >> +/* >> + * >> + * oFono - Open Source Telephony >> + * >> + * Copyright (C) 2010 Nokia Corporation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >> + * >> + */ >> + >> +/* 3GPP2 C.S0015-B v2.0, Table 3.4-1 */ >> +enum cdma_sms_tp_msg_type { >> + CDMA_SMS_P2P = 0, >> + CDMA_SMS_BCAST = 1, >> + CDMA_SMS_ACK = 2 >> +}; >> + >> +/* 3GPP2 X.S0004-550-E, Section 2.256 */ >> +enum cdma_sms_teleservice_id { >> + TELESERVICE_CMT91 = 4096, >> + TELESERVICE_WPT = 4097, >> + TELESERVICE_WMT = 4098, >> + TELESERVICE_VMN = 4099, >> + TELESERVICE_WAP = 4100, >> + TELESERVICE_WEMT = 4101, >> + TELESERVICE_SCPT = 4102, >> + TELESERVICE_CATPT = 4103 >> +}; >> + >> +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ >> +enum cdma_sms_digi_number_type { >> + CDMA_SMS_NUM_TYPE_UNKNOWN = 0, >> + CDMA_SMS_NUM_TYPE_INTERNATIONAL_NUMBER = 1, >> + CDMA_SMS_NUM_TYPE_NATIONAL_NUMBER = 2, >> + CDMA_SMS_NUM_TYPE_NETWORK_SPECIFIC_NUMBER = 3, >> + CDMA_SMS_NUM_TYPE_SUBSCRIBER_NUMBER = 4, >> + /* Reserved 5 */ >> + CDMA_SMS_NUM_TYPE_ABBREVIATED_NUMBER = 6 >> + /* Reserved 7 */ >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ >> +enum cdma_sms_data_network_number_type { >> + CDMA_SMS_NETWORK_NUM_TYPE_UNKNOWN = 0, >> + CDMA_SMS_NETWORK_TYPE_INTERNET_PROTOCOL = 1, >> + CDMA_SMS_NETWORK_TYPE_INTERNET_EMAIL_ADDRESS = 2, >> + /* All Other Values Reserved */ >> +}; >> + >> +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-3 */ >> +enum cdma_sms_numbering_plan { >> + CDMA_SMS_NUMBERING_PLAN_UNKNOWN = 0, >> + CDMA_SMS_NUMBERING_PLAN_ISDN = 1, >> + CDMA_SMS_NUMBERING_PLAN_DATA = 3, >> + CDMA_SMS_NUMBERING_PLAN_TELEX = 4, >> + CDMA_SMS_NUMBERING_PLAN_PRIVATE = 9, >> + CDMA_SMS_NUMBERING_PLAN_RESERVED = 15 >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ >> +enum cdma_sms_msg_type { >> + CDMA_SMS_RESERVED = 0, >> + CDMA_SMS_DELIVER = 1, >> + CDMA_SMS_SUBMIT = 2, >> + CDMA_SMS_CANCEL = 3, >> + CDMA_SMS_DELIVER_ACK = 4, >> + CDMA_SMS_USER_ACK = 5, >> + CDMA_SMS_READ_ACK = 6, >> +}; >> + >> +/* C.R1001-G_v1.0 Table 9.1-1 */ >> +enum cdma_sms_message_encoding { >> + MSG_ENCODING_OCTET = 0, >> + MSG_ENCODING_EXTENDED_PROTOCOL_MSG = 1, >> + MSG_ENCODING_7BIT_ASCII = 2, >> + MSG_ENCODING_IA5 = 3, >> + MSG_ENCODING_UNICODE = 4, >> + MSG_ENCODING_SHIFT_JIS = 5, >> + MSG_ENCODING_KOREAN = 6, >> + MSG_ENCODING_LATIN_HEBREW = 7, >> + MSG_ENCODING_LATIN = 8, >> + MSG_ENCODING_GSM_7BIT = 9, >> + MSG_ENCODING_GSM_DATA_CODING = 10 >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3-1 */ >> +enum cdma_sms_param_id { >> + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER = 0x00, >> + CDMA_SMS_PARAM_ID_SERVICE_CATEGORY = 0x01, >> + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS = 0x02, >> + CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS = 0x03, >> + CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS = 0x04, >> + CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS = 0x05, >> + CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION = 0x06, >> + CDMA_SMS_PARAM_ID_CAUSE_CODE = 0x07, >> + CDMA_SMS_PARAM_ID_BEARER_DATA = 0x08 >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 4.5-1 */ >> +enum cdma_sms_subparam_id { >> + CDMA_SMS_SUBPARAM_ID_MESSAGE_ID = 0x00, >> + CDMA_SMS_SUBPARAM_ID_USER_DATA = 0x01, >> + CDMA_SMS_SUBPARAM_ID_USER_RESPONSE_CODE = 0x02, >> + CDMA_SMS_SUBPARAM_ID_MC_TIME_STAMP = 0x03, >> + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_ABSOLUTE = 0x04, >> + CDMA_SMS_SUBPARAM_ID_VALIDITY_PERIOD_RELATIVE = 0x05, >> + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_ABSOLUTE = 0x06, >> + CDMA_SMS_SUBPARAM_ID_DEFERRED_DELIVERY_TIME_RELATIVE = 0x07, >> + CDMA_SMS_SUBPARAM_ID_PRIORITY_INDICATOR = 0x08, >> + CDMA_SMS_SUBPARAM_ID_PRIVACY_INDICATOR = 0x09, >> + CDMA_SMS_SUBPARAM_ID_REPLY_OPTION = 0x0A, >> + CDMA_SMS_SUBPARAM_ID_NUMBER_OF_MESSAGES = 0x0B, >> + CDMA_SMS_SUBPARAM_ID_ALERT_ON_MESSAGE_DELIVERY = 0x0C, >> + CDMA_SMS_SUBPARAM_ID_LANGUAGE_INDICATOR = 0x0D, >> + CDMA_SMS_SUBPARAM_ID_CALL_BACK_NUMBER = 0x0E, >> + CDMA_SMS_SUBPARAM_ID_MESSAGE_DISPLAY_MODE = 0x0F, >> + CDMA_SMS_SUBPARAM_ID_MULTIPLE_ENCODING_USER_DATA = 0x10, >> + CDMA_SMS_SUBPARAM_ID_MESSAGE_DEPOSIT_INDEX = 0x11, >> + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_DATA = 0x12, >> + CDMA_SMS_SUBPARAM_ID_SERVICE_CATEGORY_PROGRAM_RESULT = 0x13, >> + CDMA_SMS_SUBPARAM_ID_MESSAGE_STATUS = 0x14, >> + CDMA_SMS_SUBPARAM_ID_TP_FAILURE_CAUSE = 0x15, >> + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN = 0x16, >> + CDMA_SMS_SUBPARAM_ID_ENHANCED_VMN_ACK = 0x17 >> +}; >> + >> +/* 3GPP2 C.R1001-G Table 9.3.1-1 */ >> +enum cdma_sms_service_category { >> + SERVICE_CAT_EMERGENCY_BROADCAST = 1, >> + SERVICE_CAT_ADMINISTRATIVE = 2, >> + SERVICE_CAT_MAINTENANCE = 3, >> + SERVICE_CAT_GENERALNEWSLOCAL = 4, > > Please use the underscore some more, e.g. GENERAL_NEWS_LOCAL > Will fix. >> + SERVICE_CAT_GENERALNEWSREGIONAL = 5, >> + SERVICE_CAT_GENERALNEWSNATIONAL = 6, >> + SERVICE_CAT_GENERALNEWSINTERNATIONAL = 7, >> + SERVICE_CAT_BUSINESSFINALNEWSLOCAL = 8, >> + SERVICE_CAT_BUSINESSFINALNEWSREGIONAL = 9, >> + SERVICE_CAT_BUSINESSFINALNEWSNATIONAL = 10, >> + SERVICE_CAT_BUSINESSFINALNEWSINTNL = 11, >> + SERVICE_CAT_SPORTSNEWSLOCAL = 12, >> + SERVICE_CAT_SPORTSNEWSREGIONAL = 13, >> + SERVICE_CAT_SPORTSNEWSNATIONAL = 14, >> + SERVICE_CAT_SPORTSNEWSINTERNATIONAL = 15, >> + SERVICE_CAT_ENTERTAINMENTNEWSLOCAL = 16, >> + SERVICE_CAT_ENTERTAINMENTNEWSREGIONAL = 17, >> + SERVICE_CAT_ENTERTAINMENTNEWSNATIONAL = 18, >> + SERVICE_CAT_ENTERTAINMENTNEWSINTERNATIONAL = 19, >> + SERVICE_CAT_ENTERTAINMENTNEWSLOCALWEATHER = 20, >> + SERVICE_CAT_AREATRAFFICREPORTS = 21, >> + SERVICE_CAT_LOCALAIRTPORTFLIGHTSCHEDULES = 22, >> + SERVICE_CAT_RESTURANTS = 23, > > Is this mis-spelled? > Yep. Will fix. I am sure by now you already start wondering: what the heck is wrong with those CDMA folks, why can't they just use GSM? :-) >> + SERVICE_CAT_LODGINGS = 24, >> + SERVICE_CAT_RETAILDIRECTORYADVERTISEMENTS = 25, >> + SERVICE_CAT_ADVERTISEMENTS = 26, >> + SERVICE_CAT_STOCKQUOTES = 27, >> + SERVICE_CAT_EMPLOYMENTOPPORTUNITIES = 28, >> + SERVICE_CAT_MEDICALHEALTHHOSPITALS = 29, >> + SERVICE_CAT_TECHNOLOGYNEWS = 30, >> + SERVICE_CAT_MULTICATEGORY = 31, >> + SERVICE_CAT_CAPT = 32 >> +}; >> + >> +enum cdma_sms_digit_mode { >> + DTMF_4_BIT = 0, >> + CODE_8_BIT = 1 >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ >> +struct cdma_sms_address { >> + enum cdma_sms_digit_mode digit_mode; >> + guint8 number_mode; >> + guint8 number_type; >> + enum cdma_sms_numbering_plan number_plan; >> + guint8 num_fields; >> + char address[256]; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.6 */ >> +struct cdma_sms_cause_code { >> + guint8 reply_seq; >> + guint8 error_class; >> + guint8 cause_code; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 4.5.1 */ >> +struct cdma_sms_identifier { >> + enum cdma_sms_msg_type msg_type; >> + guint16 msg_id; >> + gboolean header_ind; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 4.5.2 */ >> +struct cdma_sms_ud { >> + enum cdma_sms_message_encoding msg_encoding; >> + guint8 num_fields; >> + guint8 chari[512]; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 4.5 */ >> +struct cdma_sms_bearer_data { >> + guint32 subparam_bitmap; >> + struct cdma_sms_identifier id; >> + struct cdma_sms_ud ud; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.1-1. */ >> +struct cdma_sms_p2p_msg { >> + guint32 param_bitmap; >> + enum cdma_sms_teleservice_id teleservice_id; >> + struct cdma_sms_address oaddr; >> + struct cdma_sms_bearer_data bearer_data; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.2-1 */ >> +struct cdma_sms_broadcast_msg { >> + enum cdma_sms_service_category service_category; >> + struct cdma_sms_bearer_data bearer_data; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.2.3-1 */ >> +struct cdma_sms_ack_msg { >> + struct cdma_sms_address daddr; >> + struct cdma_sms_cause_code cause_code; >> +}; >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 3.4.1 */ >> +struct cdma_sms { >> + enum cdma_sms_tp_msg_type type; >> + union { >> + struct cdma_sms_p2p_msg p2p_msg; >> + struct cdma_sms_broadcast_msg broadcast_msg; >> + struct cdma_sms_ack_msg ack_msg; >> + }; >> +}; >> + >> +static inline gboolean check_bitmap(guint32 bitmap, guint32 pos) >> +{ >> + guint32 mask = 0x1<< pos; >> + return bitmap& mask ? TRUE : FALSE; >> +} >> + > > I don't see this function being used by this patch. Does it logically > belong to a later patch? > Yes, it is used by a later patch of the same serie: 4/7. I will split the patch to move this part to 4/7 if you see a need. >> +static inline void set_bitmap(guint32 *bitmap, guint32 pos) >> +{ >> + if (bitmap == NULL) >> + return; >> + >> + *bitmap = *bitmap | (1<< pos); >> +} > > Why are these functions defined in the header? Are they meant to be > used elsewhere? > It is only used by cdma-smsutil.c. I will fix this by moving it into cdma-smsutil.c instead of putting it here in .h. >> + >> +gboolean cdma_sms_decode(const unsigned char *pdu, int len, >> + struct cdma_sms *out); >> +char *cdma_sms_decode_text(struct cdma_sms_ud *ud); >> +char *cdma_sms_address_to_string(const struct cdma_sms_address *addr); > > Regards, > -Denis Regards, -Lei ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support 2011-01-05 17:47 ` Lei Yu @ 2011-01-06 20:23 ` Denis Kenzior 0 siblings, 0 replies; 6+ messages in thread From: Denis Kenzior @ 2011-01-06 20:23 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 4755 bytes --] Hi Lei, >> So it looks to me like this function takes arbitrary bit fields from a >> stream. Is this a 'peculiarity' of the address field or will this be >> used elsewhere? >> > Yes, your interpretation of the function is correct. > > Actually, cross CDMA SMS spec (and also other CDMA protocol specs), a > lot of the message (parameter and etc) structures are defined just as > 'peculiar' as Address field. For example: User Data (section 4.5.2 of > C.S0015-B), Call Back Number (can be considered same as address). > > Essentially, CDMA SMS specs defines structures as bit streams and a > field can start at any offset within the bit stream. Even worse, > depending on the values of the previous fields, a field can start at > different locations within a bit stream for the different instances of > the same type of the message. > Yes, some really smart engineers must have designed this spec ;) >> This looks like a really inefficient function to call for every field, >> so it might makes things easier if the offsets were simply hardcoded... >> > > The main reason I do this way is that I feel there will be more chances > to make mistake if the 'shift', 'bit-wise and', bit-wise or' operation > is carried out when decoding each field separately. It will be cleaner > to have this generic function and decoding of each field will just care > about size of the field and start offset of the field into the stream. > I'm unconvinced. The first thing I question is why you bother decoding up to 32 bits at once in a loop. The max size of the field I've seen so far is 16 bits and even that is not common. Decoding as 8 bits and bitwise ORing in the appropriate place might be way simpler (and faster). > Efficiency-wise, I see worst case a few extra machine instructions will > be used in comparing to 'manually' decoding at each place. Efficiency wise this is actually pretty bad compared to a tight loop. Especially for things like user-data which is just 8 bits but bit-shifted a random amount. We're talking about potentially 250+ functions calls per sms segment... Since the function is fairly large, not sure if it can even be inlined successfully. We are targeting embedded devices here, right? ;) Can you try substituting a macro / inline function that can extrat a max of 8 bits from a bitstream instead? I believe such an operation would be easily inlined, would not require branching and still keep most of the advantages you list out above... >> Explicit casts should be avoided > > Will fix. Could you pls also educate me the rationales behind this rule? > Two reasons: It looks ugly; and When I'm reviewing code, every time there's an explicit cast my alarm bells go off. It could mean that one: a. Didn't think through one's data structures / code (e.g. const / non-const, enum vs int, etc). The compiler's job is to point out such possibilities in order for them to be fixed appropriately. Often casting is just an attempt to hide these warning signs. b. is doing something really evil without fully thinking it through. See point a. c. Doing something really evil for a good reason, but it is so tricky it needs special attention. This last one should be pretty rare ;) >> Have you looked at how STK pdus are decoded in stkutil.c, >> parse_dataobj() in particular. Looking at the basic code structure so >> far, that design pattern could be (or not) a really nice fit here and >> save you some kLoC in the future. >> > > Looked at it. I am assuming that the design pattern you are referring to > is that we can create an array of structure where decoding function for > a parameter record can be stored thus, this part of the code will be > reduced to something like: parsing the type and invoke the decoding > function (dataobj_handler as in stkutil.c) as in the array of the > function handlers. > > I don't see this approach and the approach I described above are really > not much differences since the logics below are nothing but a big > switch-case state which will be required even with function handler > array approach anyway. > > The only simplification I can see below is to somehow to put > set_bitmap() into one place. This will reduce ~30 LOC. > There's more to it than that. You also get automatic checking of the presence of the mandatory fields which you do not do right now and will repeat at least twice. You also don't need to code the same exact loop multiple times. That reminds me, it might be useful to have a proper iterator for subparameter data. E.g. similar to simple_tlv_iter. It should make things way easier to read for the uninitiated. Regards, -Denis ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support 2010-12-22 0:02 [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support Lei Yu 2011-01-04 21:17 ` Denis Kenzior @ 2011-01-05 22:34 ` Rajesh.Nagaiah 2011-01-07 23:47 ` Lei Yu 1 sibling, 1 reply; 6+ messages in thread From: Rajesh.Nagaiah @ 2011-01-05 22:34 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 9449 bytes --] HI Lei, My comments inline > + > +/* 3GPP2 C.S0015-B v2.0, Table 3.4-1 */ enum cdma_sms_tp_msg_type { > + CDMA_SMS_P2P = 0, > + CDMA_SMS_BCAST = 1, > + CDMA_SMS_ACK = 2 > +}; 1.Coding style M11 not followed. If enum type name is cdma_sms_tp_msg_type, then enum content should be for eg, CDMA_SMS_TP_MSG_TYPE_XXX. 2. During initial discussions with Denis, he suggested to use SMS_CDMA_XXX instead of CDMA_SMS_XXX. If he is OK with CDMA_SMS_XXX usage I am OK with it as well. > +/* 3GPP2 X.S0004-550-E, Section 2.256 */ As 3GPP2 X.S0004-550-E v4.0 Section 2.256 lists all the Teleservice types, but we are listing here only the supported teleservice types. So should we mention something like this ? /* 3GPP2 X.S0004-550-E v4.0 Section 2.256. Only supported by 3GPP2 C.S0015-B v2.0 Section 3.4.3.1 listed */ >enum cdma_sms_teleservice_id { > + TELESERVICE_CMT91 = 4096, > + TELESERVICE_WPT = 4097, > + TELESERVICE_WMT = 4098, > + TELESERVICE_VMN = 4099, > + TELESERVICE_WAP = 4100, > + TELESERVICE_WEMT = 4101, > + TELESERVICE_SCPT = 4102, > + TELESERVICE_CATPT = 4103 > +}; 1. Coding style M11 not followed. Enum content should be CDMA_SMS_TELESERVICE_ID_XXX 2. There is no coding style of the values assigned, but for such big numbers I think Hex represenation would be better. > +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ enum > +cdma_sms_digi_number_type { > + CDMA_SMS_NUM_TYPE_UNKNOWN = 0, > + CDMA_SMS_NUM_TYPE_INTERNATIONAL_NUMBER = 1, > + CDMA_SMS_NUM_TYPE_NATIONAL_NUMBER = 2, > + CDMA_SMS_NUM_TYPE_NETWORK_SPECIFIC_NUMBER = 3, > + CDMA_SMS_NUM_TYPE_SUBSCRIBER_NUMBER = 4, > + /* Reserved 5 */ > + CDMA_SMS_NUM_TYPE_ABBREVIATED_NUMBER = 6 > + /* Reserved 7 */ > +}; 1.Coding style M11 not followed. 2. _NUMBER suffix is not required. 3. Reserved values are defined in other enum declarations. So to be inline with rest of ofono code we should define those here as well > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ enum > +cdma_sms_data_network_number_type { > + CDMA_SMS_NETWORK_NUM_TYPE_UNKNOWN = 0, > + CDMA_SMS_NETWORK_TYPE_INTERNET_PROTOCOL = 1, > + CDMA_SMS_NETWORK_TYPE_INTERNET_EMAIL_ADDRESS = 2, > + /* All Other Values Reserved */ > +}; 1.Coding style M11 not followed. 2. Also some has CDMA_SMS_NETWORK_NUM_TYPE_XXX and some has CDMA_SMS_NETWORK_TYPE_XXX ? 3. Why do we need to have seperate cdma_sms_digi_number_type and cdma_sms_data_network_number_type. Rather we can merge both of it into one single enum cdma_sms_number_type, as we already know whether its digit mode or number mode in seperate parameters. For eg: /* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ and 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ enum cdma_sms_number_type { CDMA_SMS_NUMBER_TYPE_UNKNOWN = 0, CDMA_SMS_NUMBER_TYPE_INTERNATIONAL_OR_IP = 1, CDMA_SMS_NUMBER_TYPE_NATIONAL_OR_INTERNET_EMAIL = 2, CDMA_SMS_NUMBER_TYPE_NETWORK_SPECIFIC = 3, CDMA_SMS_NUMBER_TYPE_SUBSCRIBER = 4, CDMA_SMS_NUMBER_TYPE_RESERVED_5 = 5, CDMA_SMS_NUMBER_TYPE_ABBREVIATED = 6 CDMA_SMS_NUMBER_TYPE_RESERVED = 7, }; > +/* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ enum cdma_sms_msg_type { > + CDMA_SMS_RESERVED = 0, > + CDMA_SMS_DELIVER = 1, > + CDMA_SMS_SUBMIT = 2, > + CDMA_SMS_CANCEL = 3, > + CDMA_SMS_DELIVER_ACK = 4, > + CDMA_SMS_USER_ACK = 5, > + CDMA_SMS_READ_ACK = 6, > +}; 1. Coding style M11 not followed. It should be for eg, CDMA_SMS_MSG_TYPE_XXX 2. Why Deliver report and Submit report definitions missing ? Even though we dont support that feature yet in this patch, the definitions should be there. For eg: /* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ enum cdma_sms_msg_type { CDMA_SMS_MSG_TYPE_RESERVED = 0, CDMA_SMS_MSG_TYPE_DELIVER = 1, CDMA_SMS_MSG_TYPE_SUBMIT = 2, CDMA_SMS_MSG_TYPE_CANCEL = 3, CDMA_SMS_MSG_TYPE_DELIVER_ACK = 4, CDMA_SMS_MSG_TYPE_USER_ACK = 5, CDMA_SMS_MSG_TYPE_READ_ACK = 6, CDMA_SMS_MSG_TYPE_DELIVER_REPORT = 7, CDMA_SMS_MSG_TYPE_SUBMIT_REPORT = 8 }; > +/* C.R1001-G_v1.0 Table 9.1-1 */ > +enum cdma_sms_message_encoding { > + MSG_ENCODING_OCTET = 0, > + MSG_ENCODING_EXTENDED_PROTOCOL_MSG = 1, > + MSG_ENCODING_7BIT_ASCII = 2, > + MSG_ENCODING_IA5 = 3, > + MSG_ENCODING_UNICODE = 4, > + MSG_ENCODING_SHIFT_JIS = 5, > + MSG_ENCODING_KOREAN = 6, > + MSG_ENCODING_LATIN_HEBREW = 7, > + MSG_ENCODING_LATIN = 8, > + MSG_ENCODING_GSM_7BIT = 9, > + MSG_ENCODING_GSM_DATA_CODING = 10 > +}; 1. Coding style M11 not followed. For eg: /* 3GPP2 C.R1001-G v1.0 Table 9.1-1 */ enum cdma_sms_encoding_type { CDMA_SMS_ENCODING_TYPE_OCTECT_UNSPECIFIED = 0x0, CDMA_SMS_ENCODING_TYPE_EXTENDED_PROTOCOL_MESSAGE = 0x1, CDMA_SMS_ENCODING_TYPE_7BIT_ASCII = 0x2, CDMA_SMS_ENCODING_TYPE_IA5 = 0x3, CDMA_SMS_ENCODING_TYPE_UNICODE = 0x4, CDMA_SMS_ENCODING_TYPE_SHIFT_JIS = 0x5, CDMA_SMS_ENCODING_TYPE_KOREAN = 0x6, CDMA_SMS_ENCODING_TYPE_LATIN_HEBREW = 0x7, CDMA_SMS_ENCODING_TYPE_LATIN = 0x8, CDMA_SMS_ENCODING_TYPE_7BIT_GSM = 0x9, CDMA_SMS_ENCODING_TYPE_GSM_DCS = 0xA }; > +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3-1 */ enum cdma_sms_param_id { > + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER = 0x00, > + CDMA_SMS_PARAM_ID_SERVICE_CATEGORY = 0x01, > + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS = 0x02, > + CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS = 0x03, > + CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS = 0x04, > + CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS = 0x05, > + CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION = 0x06, > + CDMA_SMS_PARAM_ID_CAUSE_CODE = 0x07, > + CDMA_SMS_PARAM_ID_BEARER_DATA = 0x08 > +}; Some enum values are in plain numbering, some are in hex. We should rather follow only hex. > +/* 3GPP2 C.R1001-G Table 9.3.1-1 */ > +enum cdma_sms_service_category { > + SERVICE_CAT_EMERGENCY_BROADCAST = 1, > + SERVICE_CAT_ADMINISTRATIVE = 2, > + SERVICE_CAT_MAINTENANCE = 3, > + SERVICE_CAT_GENERALNEWSLOCAL = 4, > + SERVICE_CAT_GENERALNEWSREGIONAL = 5, > + SERVICE_CAT_GENERALNEWSNATIONAL = 6, > + SERVICE_CAT_GENERALNEWSINTERNATIONAL = 7, > + SERVICE_CAT_BUSINESSFINALNEWSLOCAL = 8, > + SERVICE_CAT_BUSINESSFINALNEWSREGIONAL = 9, > + SERVICE_CAT_BUSINESSFINALNEWSNATIONAL = 10, > + SERVICE_CAT_BUSINESSFINALNEWSINTNL = 11, > + SERVICE_CAT_SPORTSNEWSLOCAL = 12, > + SERVICE_CAT_SPORTSNEWSREGIONAL = 13, > + SERVICE_CAT_SPORTSNEWSNATIONAL = 14, > + SERVICE_CAT_SPORTSNEWSINTERNATIONAL = 15, > + SERVICE_CAT_ENTERTAINMENTNEWSLOCAL = 16, > + SERVICE_CAT_ENTERTAINMENTNEWSREGIONAL = 17, > + SERVICE_CAT_ENTERTAINMENTNEWSNATIONAL = 18, > + SERVICE_CAT_ENTERTAINMENTNEWSINTERNATIONAL = 19, > + SERVICE_CAT_ENTERTAINMENTNEWSLOCALWEATHER = 20, > + SERVICE_CAT_AREATRAFFICREPORTS = 21, > + SERVICE_CAT_LOCALAIRTPORTFLIGHTSCHEDULES = 22, > + SERVICE_CAT_RESTURANTS = 23, > + SERVICE_CAT_LODGINGS = 24, > + SERVICE_CAT_RETAILDIRECTORYADVERTISEMENTS = 25, > + SERVICE_CAT_ADVERTISEMENTS = 26, > + SERVICE_CAT_STOCKQUOTES = 27, > + SERVICE_CAT_EMPLOYMENTOPPORTUNITIES = 28, > + SERVICE_CAT_MEDICALHEALTHHOSPITALS = 29, > + SERVICE_CAT_TECHNOLOGYNEWS = 30, > + SERVICE_CAT_MULTICATEGORY = 31, > + SERVICE_CAT_CAPT = 32 > +}; 1. Coding style M11 not followed. 2. In SERVICE_CAT_XXX the XXX part is completely unreadable. for eg: change _ENTERTAINMENTNEWSLOCALWEATHER to _ENTERTAINMENT_NEWS_LOCAL_WEATHER 3. CMAS Broadcast Service Category Assignments definitions are missing Include the following as well: CDMA_SMS_SERVICE_CATEGORY_PRESIDENTIAL_LEVEL_ALERT = 0x1000, CDMA_SMS_SERVICE_CATEGORY_EXTREME_THREAT_TO_LIFE_AND_PROPERTY = 0x1001, CDMA_SMS_SERVICE_CATEGORY_SEVERE_THREAT_TO_LIFE_AND_PROPERTY = 0x1002, CDMA_SMS_SERVICE_CATEGORY_AMBER_CHILD_ABDUCTION_EMERGENCY = 0x1003, CDMA_SMS_SERVICE_CATEGORY_CMAS_TEST_MESSAGE = 0x1004 > +enum cdma_sms_digit_mode { > + DTMF_4_BIT = 0, > + CODE_8_BIT = 1 > +}; 1. Coding style M11 not followed. 2. CODE_8_BIT sounds odd, ASCII gives more meaning to it. 3. 3GPP2 reference missing. change to: /* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ enum cdma_sms_digit_mode_type { CDMA_SMS_DIGIT_MODE_TYPE_4BIT_DTMF = 0x0, CDMA_SMS_DIGIT_MODE_TYPE_8BIT_ASCII = 0x1 }; For digit mode type its defined as enum, but no enum defintion for number mode type. So add, /* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ enum cdma_sms_number_mode_type { CDMA_SMS_NUMBER_MODE_TYPE_NON_DATA_NETWORK_ADDRESS = 0x0, CDMA_SMS_NUMBER_MODE_TYPE_DATA_NETWORK_ADDRESS = 0x1 }; > + > +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ struct cdma_sms_address { > + enum cdma_sms_digit_mode digit_mode; > + guint8 number_mode; > + guint8 number_type; > + enum cdma_sms_numbering_plan number_plan; > + guint8 num_fields; > + char address[256]; > +}; 1.Some members are defined as enums, some as guint8 eventhough they have corresponding enum defintions. This will lead to explicit casts in the code later. So we should chnage that. 2. How did we get 256 as max address length ? Also avoid using magic numbers, rather declare them as constants. > +/* 3GPP2 C.S0015-B v2.0 Section 4.5.2 */ struct cdma_sms_ud { > + enum cdma_sms_message_encoding msg_encoding; > + guint8 num_fields; > + guint8 chari[512]; > +}; 1. Again how did we get 512 as max UD length ? Also avoid using magic numbers, rather declare them as constants. BR, Rajesh ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support 2011-01-05 22:34 ` Rajesh.Nagaiah @ 2011-01-07 23:47 ` Lei Yu 0 siblings, 0 replies; 6+ messages in thread From: Lei Yu @ 2011-01-07 23:47 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 12582 bytes --] Hi Rajesh, On 01/05/2011 02:34 PM, ext Rajesh.Nagaiah(a)elektrobit.com wrote: > HI Lei, > > My comments inline > >> + >> +/* 3GPP2 C.S0015-B v2.0, Table 3.4-1 */ enum cdma_sms_tp_msg_type { >> + CDMA_SMS_P2P = 0, >> + CDMA_SMS_BCAST = 1, >> + CDMA_SMS_ACK = 2 >> +}; > > 1.Coding style M11 not followed. > If enum type name is cdma_sms_tp_msg_type, then enum content > should be for eg, CDMA_SMS_TP_MSG_TYPE_XXX. Will fix. > 2. During initial discussions with Denis, he suggested to use > SMS_CDMA_XXX instead of CDMA_SMS_XXX. If he is OK with CDMA_SMS_XXX > usage I am OK with it as well. > Checked with Denis, will stick with CDMA_SMS_XXX. >> +/* 3GPP2 X.S0004-550-E, Section 2.256 */ > > As 3GPP2 X.S0004-550-E v4.0 Section 2.256 lists all the Teleservice > types, but we are listing here only the supported teleservice types. > So should we mention something like this ? > > /* 3GPP2 X.S0004-550-E v4.0 Section 2.256. > Only supported by 3GPP2 C.S0015-B v2.0 Section 3.4.3.1 listed */ > Will fix. >> enum cdma_sms_teleservice_id { >> + TELESERVICE_CMT91 = 4096, >> + TELESERVICE_WPT = 4097, >> + TELESERVICE_WMT = 4098, >> + TELESERVICE_VMN = 4099, >> + TELESERVICE_WAP = 4100, >> + TELESERVICE_WEMT = 4101, >> + TELESERVICE_SCPT = 4102, >> + TELESERVICE_CATPT = 4103 >> +}; > > 1. Coding style M11 not followed. > Enum content should be CDMA_SMS_TELESERVICE_ID_XXX > Will fix. > 2. There is no coding style of the values assigned, but > for such big numbers I think Hex represenation would be > better. > I would prefer sticking with the value defined in the spec which is non-hex. Easier for reader to verify. >> +/* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ enum >> +cdma_sms_digi_number_type { >> + CDMA_SMS_NUM_TYPE_UNKNOWN = 0, >> + CDMA_SMS_NUM_TYPE_INTERNATIONAL_NUMBER = 1, >> + CDMA_SMS_NUM_TYPE_NATIONAL_NUMBER = 2, >> + CDMA_SMS_NUM_TYPE_NETWORK_SPECIFIC_NUMBER = 3, >> + CDMA_SMS_NUM_TYPE_SUBSCRIBER_NUMBER = 4, >> + /* Reserved 5 */ >> + CDMA_SMS_NUM_TYPE_ABBREVIATED_NUMBER = 6 >> + /* Reserved 7 */ >> +}; > > 1.Coding style M11 not followed. Will fix. > 2. _NUMBER suffix is not required. Will fix. > 3. Reserved values are defined in other enum declarations. > So to be inline with rest of ofono code we should define those > here as well > Will follow rest ofono. >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ enum >> +cdma_sms_data_network_number_type { >> + CDMA_SMS_NETWORK_NUM_TYPE_UNKNOWN = 0, >> + CDMA_SMS_NETWORK_TYPE_INTERNET_PROTOCOL = 1, >> + CDMA_SMS_NETWORK_TYPE_INTERNET_EMAIL_ADDRESS = 2, >> + /* All Other Values Reserved */ >> +}; > > 1.Coding style M11 not followed. Will fix. > 2. Also some has CDMA_SMS_NETWORK_NUM_TYPE_XXX > and some has CDMA_SMS_NETWORK_TYPE_XXX ? Will fix. > 3. Why do we need to have seperate cdma_sms_digi_number_type and > cdma_sms_data_network_number_type. Rather we can merge both of > it into one single enum cdma_sms_number_type, as we already know > whether its digit mode or number mode in seperate parameters. > For eg: > /* 3GPP2 C.S0005-E v2.0 Table 2.7.1.3.2.4-2 */ > and 3GPP2 C.S0015-B v2.0 Table 3.4.3.3-1 */ > enum cdma_sms_number_type { > CDMA_SMS_NUMBER_TYPE_UNKNOWN = 0, > CDMA_SMS_NUMBER_TYPE_INTERNATIONAL_OR_IP = 1, > CDMA_SMS_NUMBER_TYPE_NATIONAL_OR_INTERNET_EMAIL = 2, > CDMA_SMS_NUMBER_TYPE_NETWORK_SPECIFIC = 3, > CDMA_SMS_NUMBER_TYPE_SUBSCRIBER = 4, > CDMA_SMS_NUMBER_TYPE_RESERVED_5 = 5, > CDMA_SMS_NUMBER_TYPE_ABBREVIATED = 6 > CDMA_SMS_NUMBER_TYPE_RESERVED = 7, > }; > > Having two separate enums, each matching with one table in the standard will be much cleaner. Otherwise, it will be hard for reader to check where those number from. >> +/* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ enum cdma_sms_msg_type { >> + CDMA_SMS_RESERVED = 0, >> + CDMA_SMS_DELIVER = 1, >> + CDMA_SMS_SUBMIT = 2, >> + CDMA_SMS_CANCEL = 3, >> + CDMA_SMS_DELIVER_ACK = 4, >> + CDMA_SMS_USER_ACK = 5, >> + CDMA_SMS_READ_ACK = 6, >> +}; > > 1. Coding style M11 not followed. > It should be for eg, CDMA_SMS_MSG_TYPE_XXX Will fix. > 2. Why Deliver report and Submit report definitions missing ? > Even though we dont support that feature yet in this patch, > the definitions should be there. > > For eg: > /* 3GPP2 C.S0015-B v2.0 Table 4.5.1-1 */ > enum cdma_sms_msg_type { > CDMA_SMS_MSG_TYPE_RESERVED = 0, > CDMA_SMS_MSG_TYPE_DELIVER = 1, > CDMA_SMS_MSG_TYPE_SUBMIT = 2, > CDMA_SMS_MSG_TYPE_CANCEL = 3, > CDMA_SMS_MSG_TYPE_DELIVER_ACK = 4, > CDMA_SMS_MSG_TYPE_USER_ACK = 5, > CDMA_SMS_MSG_TYPE_READ_ACK = 6, > CDMA_SMS_MSG_TYPE_DELIVER_REPORT = 7, > CDMA_SMS_MSG_TYPE_SUBMIT_REPORT = 8 > }; > Will fix. >> +/* C.R1001-G_v1.0 Table 9.1-1 */ >> +enum cdma_sms_message_encoding { >> + MSG_ENCODING_OCTET = 0, >> + MSG_ENCODING_EXTENDED_PROTOCOL_MSG = 1, >> + MSG_ENCODING_7BIT_ASCII = 2, >> + MSG_ENCODING_IA5 = 3, >> + MSG_ENCODING_UNICODE = 4, >> + MSG_ENCODING_SHIFT_JIS = 5, >> + MSG_ENCODING_KOREAN = 6, >> + MSG_ENCODING_LATIN_HEBREW = 7, >> + MSG_ENCODING_LATIN = 8, >> + MSG_ENCODING_GSM_7BIT = 9, >> + MSG_ENCODING_GSM_DATA_CODING = 10 >> +}; > > 1. Coding style M11 not followed. > For eg: > /* 3GPP2 C.R1001-G v1.0 Table 9.1-1 */ > enum cdma_sms_encoding_type { > CDMA_SMS_ENCODING_TYPE_OCTECT_UNSPECIFIED = 0x0, > CDMA_SMS_ENCODING_TYPE_EXTENDED_PROTOCOL_MESSAGE = 0x1, > CDMA_SMS_ENCODING_TYPE_7BIT_ASCII = 0x2, > CDMA_SMS_ENCODING_TYPE_IA5 = 0x3, > CDMA_SMS_ENCODING_TYPE_UNICODE = 0x4, > CDMA_SMS_ENCODING_TYPE_SHIFT_JIS = 0x5, > CDMA_SMS_ENCODING_TYPE_KOREAN = 0x6, > CDMA_SMS_ENCODING_TYPE_LATIN_HEBREW = 0x7, > CDMA_SMS_ENCODING_TYPE_LATIN = 0x8, > CDMA_SMS_ENCODING_TYPE_7BIT_GSM = 0x9, > CDMA_SMS_ENCODING_TYPE_GSM_DCS = 0xA > }; Will fix. > >> +/* 3GPP2 C.S0015-B v2.0 Table 3.4.3-1 */ enum cdma_sms_param_id { >> + CDMA_SMS_PARAM_ID_TELESERVICE_IDENTIFIER = 0x00, >> + CDMA_SMS_PARAM_ID_SERVICE_CATEGORY = 0x01, >> + CDMA_SMS_PARAM_ID_ORIGINATING_ADDRESS = 0x02, >> + CDMA_SMS_PARAM_ID_ORIGINATING_SUBADDRESS = 0x03, >> + CDMA_SMS_PARAM_ID_DESTINATION_ADDRESS = 0x04, >> + CDMA_SMS_PARAM_ID_DESTINATION_SUBADDRESS = 0x05, >> + CDMA_SMS_PARAM_ID_BEARER_REPLY_OPTION = 0x06, >> + CDMA_SMS_PARAM_ID_CAUSE_CODE = 0x07, >> + CDMA_SMS_PARAM_ID_BEARER_DATA = 0x08 >> +}; > > Some enum values are in plain numbering, some are in hex. > We should rather follow only hex. > Will fix. >> +/* 3GPP2 C.R1001-G Table 9.3.1-1 */ >> +enum cdma_sms_service_category { >> + SERVICE_CAT_EMERGENCY_BROADCAST = 1, >> + SERVICE_CAT_ADMINISTRATIVE = 2, >> + SERVICE_CAT_MAINTENANCE = 3, >> + SERVICE_CAT_GENERALNEWSLOCAL = 4, >> + SERVICE_CAT_GENERALNEWSREGIONAL = 5, >> + SERVICE_CAT_GENERALNEWSNATIONAL = 6, >> + SERVICE_CAT_GENERALNEWSINTERNATIONAL = 7, >> + SERVICE_CAT_BUSINESSFINALNEWSLOCAL = 8, >> + SERVICE_CAT_BUSINESSFINALNEWSREGIONAL = 9, >> + SERVICE_CAT_BUSINESSFINALNEWSNATIONAL = 10, >> + SERVICE_CAT_BUSINESSFINALNEWSINTNL = 11, >> + SERVICE_CAT_SPORTSNEWSLOCAL = 12, >> + SERVICE_CAT_SPORTSNEWSREGIONAL = 13, >> + SERVICE_CAT_SPORTSNEWSNATIONAL = 14, >> + SERVICE_CAT_SPORTSNEWSINTERNATIONAL = 15, >> + SERVICE_CAT_ENTERTAINMENTNEWSLOCAL = 16, >> + SERVICE_CAT_ENTERTAINMENTNEWSREGIONAL = 17, >> + SERVICE_CAT_ENTERTAINMENTNEWSNATIONAL = 18, >> + SERVICE_CAT_ENTERTAINMENTNEWSINTERNATIONAL = 19, >> + SERVICE_CAT_ENTERTAINMENTNEWSLOCALWEATHER = 20, >> + SERVICE_CAT_AREATRAFFICREPORTS = 21, >> + SERVICE_CAT_LOCALAIRTPORTFLIGHTSCHEDULES = 22, >> + SERVICE_CAT_RESTURANTS = 23, >> + SERVICE_CAT_LODGINGS = 24, >> + SERVICE_CAT_RETAILDIRECTORYADVERTISEMENTS = 25, >> + SERVICE_CAT_ADVERTISEMENTS = 26, >> + SERVICE_CAT_STOCKQUOTES = 27, >> + SERVICE_CAT_EMPLOYMENTOPPORTUNITIES = 28, >> + SERVICE_CAT_MEDICALHEALTHHOSPITALS = 29, >> + SERVICE_CAT_TECHNOLOGYNEWS = 30, >> + SERVICE_CAT_MULTICATEGORY = 31, >> + SERVICE_CAT_CAPT = 32 >> +}; > > 1. Coding style M11 not followed. Will fix, > 2. In SERVICE_CAT_XXX the XXX part is completely unreadable. > for eg: > change _ENTERTAINMENTNEWSLOCALWEATHER > to _ENTERTAINMENT_NEWS_LOCAL_WEATHER Will Fix. > 3. CMAS Broadcast Service Category Assignments definitions are missing > Include the following as well: > CDMA_SMS_SERVICE_CATEGORY_PRESIDENTIAL_LEVEL_ALERT = 0x1000, > CDMA_SMS_SERVICE_CATEGORY_EXTREME_THREAT_TO_LIFE_AND_PROPERTY = > 0x1001, > CDMA_SMS_SERVICE_CATEGORY_SEVERE_THREAT_TO_LIFE_AND_PROPERTY = > 0x1002, > CDMA_SMS_SERVICE_CATEGORY_AMBER_CHILD_ABDUCTION_EMERGENCY = > 0x1003, > CDMA_SMS_SERVICE_CATEGORY_CMAS_TEST_MESSAGE = 0x1004 > Will fix. >> +enum cdma_sms_digit_mode { >> + DTMF_4_BIT = 0, >> + CODE_8_BIT = 1 >> +}; > > 1. Coding style M11 not followed. Will fix. > 2. CODE_8_BIT sounds odd, ASCII gives more meaning to it. Will change to 8_BIT_ASCII since ASCII can also be 7-Bit. > 3. 3GPP2 reference missing. > > change to: > /* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ > enum cdma_sms_digit_mode_type { > CDMA_SMS_DIGIT_MODE_TYPE_4BIT_DTMF = 0x0, > CDMA_SMS_DIGIT_MODE_TYPE_8BIT_ASCII = 0x1 > }; > > For digit mode type its defined as enum, but no enum defintion > for number mode type. > > So add, > > /* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ > enum cdma_sms_number_mode_type { > CDMA_SMS_NUMBER_MODE_TYPE_NON_DATA_NETWORK_ADDRESS = 0x0, > CDMA_SMS_NUMBER_MODE_TYPE_DATA_NETWORK_ADDRESS = 0x1 > }; > Will fix. >> + >> +/* 3GPP2 C.S0015-B v2.0 Section 3.4.3.3 */ struct cdma_sms_address { >> + enum cdma_sms_digit_mode digit_mode; >> + guint8 number_mode; >> + guint8 number_type; >> + enum cdma_sms_numbering_plan number_plan; >> + guint8 num_fields; >> + char address[256]; >> +}; > > 1.Some members are defined as enums, some as guint8 eventhough they have > corresponding enum defintions. This will lead to explicit casts in the > code later. So we should chnage that. Will fix. > 2. How did we get 256 as max address length ? > Also avoid using magic numbers, rather declare them as constants. > The max of 256 is because num_field is only 8-bits. I see existing ofono code using hard coded numbers all over the places, Any rule for that? Will define constant. >> +/* 3GPP2 C.S0015-B v2.0 Section 4.5.2 */ struct cdma_sms_ud { >> + enum cdma_sms_message_encoding msg_encoding; >> + guint8 num_fields; >> + guint8 chari[512]; >> +}; > > 1. Again how did we get 512 as max UD length ? > Also avoid using magic numbers, rather declare them as constants. > chari in UD can be maximum of 512-bytes long. num_fields is max of 256 and each chari is max of 2 bytes. Again, existing ofono using hard coded values. I will define constant. > BR, > Rajesh > _______________________________________________ > ofono mailing list > ofono(a)ofono.org > http://lists.ofono.org/listinfo/ofono regards, Lei ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-07 23:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-22 0:02 [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support Lei Yu 2011-01-04 21:17 ` Denis Kenzior 2011-01-05 17:47 ` Lei Yu 2011-01-06 20:23 ` Denis Kenzior 2011-01-05 22:34 ` Rajesh.Nagaiah 2011-01-07 23:47 ` Lei Yu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.