Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support

[Lei Yu <lei.2.yu@nokia.com>]

[-- 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