Hi Rajesh,
On 01/05/2011 02:34 PM, ext rajesh.naga...@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@ofono.org
http://lists.ofono.org/listinfo/ofono
regards,
Lei
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
