**** BEGIN LOGGING AT Wed Mar 17 02:59:57 2010
Mar 17 03:07:59 <ygu5_ubuntu> denkenz: ping
Mar 17 03:14:23 <denkenz> pong
Mar 17 03:34:48 <ygu5_ubuntu> denkenz: I have bunch of questions here;)
Mar 17 03:35:07 <ygu5_ubuntu> 1. Does at command contain '\0\?
Mar 17 03:35:16 <ygu5_ubuntu> '\0'
Mar 17 03:41:28 <denkenz> shouldn't
Mar 17 03:44:14 <ygu5_ubuntu> OK, so we don't need len for it. 
Mar 17 03:44:23 <ygu5_ubuntu> 2. Could DF have more than 2 levels? Can I still use GSList to store a file list? We can also have a pointer array (maximum size is 256) to store it, but I think in this way, we may consume much more memory.
Mar 17 03:50:11 <denkenz> So as I pointed in my reply, we should ignore the possibility of more than 2 DFs
Mar 17 03:50:21 <denkenz> If that turns out to be wrong, we fix it later
Mar 17 03:50:37 <denkenz> GSList is fine though
Mar 17 03:52:00 <ygu5_ubuntu> OK
Mar 17 03:52:08 <ygu5_ubuntu> 3. Why is utf8 defined as char *, rather than unsigned char *?
Mar 17 03:53:47 <denkenz> more convenience, ascii is subset of utf8
Mar 17 03:54:02 <denkenz> and most printf* functions handle utf8
Mar 17 03:54:54 <ygu5_ubuntu> got it.
Mar 17 03:54:55 <ygu5_ubuntu> 4. In current implementaion of parse_send_sms(), it seems the case for CDMA is not handled. Do we ignore it for the time being?
Mar 17 03:55:16 <denkenz> for now yes, we can't handle CDMA ones anyway
Mar 17 03:55:37 <denkenz> maybe check that gsm pdu was actually given and fail parsing otherwise
Mar 17 03:56:24 <ygu5_ubuntu> If so, I also think we should change it to be DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM
Mar 17 03:56:53 <denkenz> That's fine for now
Mar 17 03:57:09 <denkenz> Though it is better to handle it as a special case
Mar 17 03:57:30 <denkenz> Since its not really mandatory
Mar 17 03:57:50 <ygu5_ubuntu> Would oFono support CDMA someday?
Mar 17 03:57:55 <denkenz> yes
Mar 17 03:58:05 <ygu5_ubuntu> Then we can handle it then. 
Mar 17 03:58:14 <ygu5_ubuntu> 5. Need we know the difference： some comprehension tlv is absent, or the ctlv is there and have default value.
Mar 17 03:58:56 <denkenz> generally this is not a problem, if a ctlv is absent it can be marked null
Mar 17 03:59:11 <denkenz> Only a few special cases like display text having null values
Mar 17 04:01:47 <ygu5_ubuntu> Fine. 
Mar 17 04:02:08 <ygu5_ubuntu> Last question is about the logic in parse_dataobj()
Mar 17 04:02:50 <ygu5_ubuntu> If some ctlv doesn't have valid content, the whole ber-tlv would not be decoded.
Mar 17 04:03:06 <ygu5_ubuntu> Is that an expected behavior?
Mar 17 04:04:05 <denkenz> That's not really how it works
Mar 17 04:04:43 <ygu5_ubuntu> I mean the ret is FALSE. and comprehension_tlv_iter_next(iter) will never be executed.
Mar 17 04:04:48 <ygu5_ubuntu> Am I wrong?
Mar 17 04:06:14 <denkenz> Basically you're correct, but we do retry other ctlv types
Mar 17 04:06:57 <denkenz> So if the data inside the ctlv is corrupt, then yes we fail to parse it
Mar 17 04:07:00 <ygu5_ubuntu> But iter never goes forward, so the rest handing would all fail.
Mar 17 04:07:44 <ygu5_ubuntu> I'm OK for it right now, if it's an expected behavior.
Mar 17 04:08:01 <ygu5_ubuntu> I just think this's too strict.
Mar 17 04:08:35 <denkenz> I have to check what is expected if the data is invalid, I believe we should fail
Mar 17 04:08:49 <denkenz> But I have been wrong before
Mar 17 04:09:31 <denkenz> Perhaps we need a different return type depending on whether it is a tag mismatch or the corrupt data
Mar 17 04:09:42 <denkenz> Then if corrupt we can advance the iter
Mar 17 04:10:09 <ygu5_ubuntu> A simple alternative is to check the tag of each iter, and handle it.
Mar 17 04:10:31 <ygu5_ubuntu> But this also has a drawback: It's hard to check the sequence.
Mar 17 04:12:06 <ygu5_ubuntu> Anyway, if we want to have the logic to check the validity of the ber-tlv, the ctlv, the sequence, the M|O|C, minimum or not, the code could be too complicated.
Mar 17 04:13:15 <ygu5_ubuntu> So now, we could be strict, and make code simple.
Mar 17 04:14:34 <ygu5_ubuntu> Your idea on different return type is also a good enhancement.
Mar 17 04:15:34 <denkenz> The minimum part is easy, but we have to enforce the sequence, the standard mandates it
Mar 17 04:15:57 <denkenz> anyway, feel free to suggest improvements, you have to do most of the hard work anyway ;)
Mar 17 04:18:15 <ygu5_ubuntu> For now, I'd like to leave it be. I will think about it after the basic things can work.
Mar 17 04:18:31 <ygu5_ubuntu> Thanks for your answer.
Mar 17 04:19:06 <ygu5_ubuntu> have a good night!
Mar 17 04:33:24 <holtmann> denkenz: Do you have any problems with the PPP filenames?
Mar 17 04:34:01 <denkenz> yes, I'd rather they not be called gatfooprotocol
Mar 17 04:34:09 <denkenz> particularly if they're internal
Mar 17 04:37:44 <denkenz> I'd also prefer ppp_fooprotocol and the internal header to be named ppp instead of foo_internal.h
Mar 17 04:38:06 <denkenz> Then again, much of what we have now is not terribly consistent...
Mar 17 04:46:37 <holtmann> I think we are not really getting around the gatppp<proto>.c thing.
Mar 17 04:46:52 <holtmann> The _internal is something I dislike too.
Mar 17 04:47:01 <denkenz> imo its fine to call them ppp_proto.c
Mar 17 04:47:10 <denkenz> Then leave gatppp.c as is
Mar 17 04:47:26 <holtmann> Yeah. I think ppp_auth.c etc. makes sense.
Mar 17 04:47:35 <holtmann> We do the same for gsm0710.c
Mar 17 04:47:47 <holtmann> And ringbuffer.c
Mar 17 04:47:52 <denkenz> exactly, and we can name the internal header ppp.h
Mar 17 04:48:03 <holtmann> Only the ones that are exported have gat* prefix.
Mar 17 04:48:29 <denkenz> Just make sure the internal ones are not installed, and we might have to fixup ringbuffer.h since it has include guards
Mar 17 04:48:46 <holtmann> Hah. I overlooked that one.
Mar 17 04:48:59 <holtmann> My mistake.
Mar 17 04:49:07 <denkenz> Was probably mine actually
Mar 17 04:49:18 <holtmann> So does gsm0710.h as well :(
Mar 17 04:49:19 <denkenz> Same with gsm0710.h
Mar 17 04:49:40 <holtmann> I am bad here. So I have to do some cleanup.
Mar 17 04:49:45 <denkenz> Again, I think we need to start thinking of making gatchat into a proper lib with subdirectories
Mar 17 04:49:55 <denkenz> e.g. src, tools
Mar 17 04:50:00 <denkenz> include
Mar 17 04:50:04 <holtmann> Until we are done with PPP and AT server, I don't wanna go there.
Mar 17 04:50:22 <holtmann> But yes, I general I agree. Other projects would benefit from it a lot.
Mar 17 04:50:37 <denkenz> Well, for now we don't have to break it out
Mar 17 04:50:46 <denkenz> But internally we can start moving stuff around
Mar 17 04:50:57 <denkenz> Especially since no other project includes it now
Mar 17 04:51:04 <holtmann> I like to not do it.
Mar 17 04:51:08 <holtmann> At least not now.
Mar 17 04:51:29 <holtmann> Good that ConnMan is gatchat free these days :)
Mar 17 04:51:39 <denkenz> That's fine, not urgent, but it will need major cleanup eventually
Mar 17 04:52:04 <denkenz> We are just running too fast lately, it is starting to catch up with us
Mar 17 04:53:39 <denkenz> And I'm fine if you push Kristen's changes and just git mv
Mar 17 04:54:17 <holtmann> No. I will ask here to rename. And then I merge it.
Mar 17 04:54:31 <denkenz> Then we better do one more round of reviews
Mar 17 04:54:32 <holtmann> These are quick changes on here side.
Mar 17 04:54:42 <holtmann> I go through it in an hour.
Mar 17 04:54:54 <denkenz> Ok, I won't do it until tomorrow morning
Mar 17 04:55:28 <holtmann> Don't worry. I do the first review. You take care of your other 100+ patches.
Mar 17 04:58:53 <denkenz> I do want to implement the ppp gprs context by end of this month too
Mar 17 04:59:15 <denkenz> So I should at least review gatppp anyway
Mar 17 14:06:34 <ygu5_home> denkenz: around?
Mar 17 15:05:10 <denkenz> ygu5_home: yep
Mar 17 15:05:44 <ygu5_home> denkenz: Sorry to disturb you again in the morning.
Mar 17 15:06:06 <ygu5_home> Can we discuss the patch of parse_dataobj_file_list()?
Mar 17 15:06:54 <ygu5_home> I think I don't need to know the identify of file except 0x3f.
Mar 17 15:07:20 <ygu5_home> Take an example: 3F00 7F10 6F3A 3F00 2FE2
Mar 17 15:07:28 <ygu5_home> Here are two files.
Mar 17 15:07:37 <ygu5_home> Is that correct? 
Mar 17 15:08:06 <ygu5_home> Both begin with 0x3F
Mar 17 15:08:25 <denkenz> yep, but 0x3F can occur in 2nd byte
Mar 17 15:09:34 <denkenz> But yes, in theory you can get away with just 0x3F, still, to be more pedantic it might be worthwhile to do a mini-parser that verifies the file structure
Mar 17 15:09:41 <ygu5_home> But the spec says "each entry in the file description is composed of two bytes, where the first byte identifies the type of file"
Mar 17 15:10:04 <denkenz> Well, e.g. this can happen: 3F00 7F10 6F3F
Mar 17 15:10:33 <ygu5_home> I don't think the second 0x3F is the beginning of a file.
Mar 17 15:10:38 <denkenz> it isn't
Mar 17 15:10:51 <ygu5_home> So my code add i by 2 every time.
Mar 17 15:10:58 <ygu5_home> Here is the code:
Mar 17 15:11:03 <ygu5_home> for (i = start + 4; i <= len; i += 2) {
Mar 17 15:11:04 <ygu5_home> 	if ((data[i] == 0x3f) || (i == len)) {
Mar 17 15:11:04 <ygu5_home> 		sf = g_new0(struct stk_file, 1);
Mar 17 15:11:04 <ygu5_home> 		sf->file = g_malloc(i-start);
Mar 17 15:11:04 <ygu5_home> 		memcpy(sf->file, data+start, i-start);
Mar 17 15:11:04 <ygu5_home> 		sf->len = i - start;
Mar 17 15:11:04 <ygu5_home> 		*fl = g_slist_prepend(*fl, sf);
Mar 17 15:11:05 <ygu5_home> 		start = i;
Mar 17 15:11:05 <ygu5_home> 	}
Mar 17 15:11:06 <ygu5_home> }
Mar 17 15:11:24 <ygu5_home> Just to skip this situation
Mar 17 15:11:44 <denkenz> start + 4 makes no sense, that is not where the filenames start
Mar 17 15:12:33 <ygu5_home> I check data[start] has to be 0x3F
Mar 17 15:12:51 <ygu5_home> And every file should be at least 4 bytes.
Mar 17 15:12:56 <denkenz> except the filenames start at data + 1
Mar 17 15:12:59 <ygu5_home> So I begin with start + 4
Mar 17 15:13:32 <ygu5_home> > +	if (data[start] != 0x3f) > +		return FALSE;
Mar 17 15:13:56 <ygu5_home> I check the first byte has to be 0x3F
Mar 17 15:14:11 <ygu5_home> The for loop is to find the end of each file path
Mar 17 15:14:21 <ygu5_home> So I begin from start + 4
Mar 17 15:14:49 <denkenz> again, I suggest you use the TS 11.11 semantics to do the checking, and create the file once elementary file id is reached
Mar 17 15:15:07 <denkenz> Relying on 0x3F is not good enough
Mar 17 15:15:39 <denkenz> Your goal here is not to submit something that barely works, but submit something that is bullet proof
Mar 17 15:16:11 <denkenz> So I don't care if it takes you two weeks instead of one, if the quality is there
Mar 17 15:16:13 <ygu5_home> OK. I will do that.
Mar 17 15:17:37 <ygu5_home> You have listed 6 types for MF|DF|EF. May I check the validity based on these?
Mar 17 15:17:52 <denkenz> yeah, check based on those
Mar 17 15:17:58 <ygu5_home> OK.
Mar 17 15:18:13 <ygu5_home> I will do that tomorrow, and send you the patch for review.
Mar 17 15:18:54 <ygu5_home> Another problem for me is to parse ber-tlv open channel.
Mar 17 15:19:23 <ygu5_home> It has many different sub-formats
Mar 17 15:19:44 <ygu5_home> I don't have a good idea to parse it.
Mar 17 15:19:57 <ygu5_home> I can have several structs to define each of them.
Mar 17 15:20:11 <ygu5_home> And use an union to include these structs.
Mar 17 15:20:51 <denkenz> that would be my approach
Mar 17 15:21:11 <ygu5_home> But how to parse them?
Mar 17 15:21:20 <ygu5_home> Use several if else?
Mar 17 15:22:41 <denkenz> How does it distinguish between the different types? qualifier inside command details or ?
Mar 17 15:22:57 <ygu5_home> That's my question too;)
Mar 17 15:23:16 <ygu5_home> I looked at command details, but didn't find the clue.
Mar 17 15:24:06 <denkenz> Dig some more and check the test spec
Mar 17 15:24:20 <denkenz> Sometimes looking at sample data makes things clearer
Mar 17 15:24:24 <ygu5_home> OK. I will search the spec again.
Mar 17 15:24:46 <ygu5_home> Good suggestion, and I will.
Mar 17 15:26:22 <ygu5_home> Have a good day:)
Mar 17 15:37:10 <denkenz> I need to run away and hide from all the patches now
Mar 18 00:37:03 <zhenhua> denkenz: ping
Mar 18 00:54:47 <denkenz> zhenhua: pong
Mar 18 00:56:09 <zhenhua> denkenz: i am not convinced about caching result in send_final.
Mar 18 00:56:27 <zhenhua> the current way looks good and clean enough to me.
Mar 18 00:56:39 <denkenz> and doesn't handle extended results
Mar 18 00:57:18 <zhenhua> sorry? what do you mean?
Mar 18 00:57:45 <denkenz> The code doesn't handle extended errors right now
Mar 18 00:58:47 <denkenz> And I don't see the point of the callback doing something like:
Mar 18 00:58:48 <zhenhua> the extended error is sent from g_at_server_send_ext_final by upper layer. the callback should return EXT_ERROR then we're done.
Mar 18 00:59:03 <denkenz> send_ext_error; return EXT_ERROR
Mar 18 01:00:39 <zhenhua> i think no need to have it. the callback should return EXT_ERROR. no need to expose send_ext_final api to upper layer.
Mar 18 01:01:17 <denkenz> And how do you propose to send +CME ERROR?
Mar 18 01:02:05 <zhenhua> you're right. i should add this api.
Mar 18 01:02:25 <zhenhua> i forgot it.
Mar 18 01:03:06 <zhenhua> anyway, my concern is: to cache the result makes send_final more complex
Mar 18 01:03:21 <denkenz> You're only caching the last one
Mar 18 01:03:41 <denkenz> And in fact you don't need to even format the response unless its an extended response
Mar 18 01:04:31 <denkenz> I bet ya it isn't even that complex ;)
Mar 18 01:04:52 <zhenhua> how do you know when it is the last OK response?
Mar 18 01:05:22 <denkenz> When you finished parsing the command line
Mar 18 01:06:04 <zhenhua> i know, but you invoke the same send_final. the last and current result is all OK
Mar 18 01:06:37 <zhenhua> you wanna add  something send_flush?
Mar 18 01:06:39 <denkenz> what the application invokes will be different from what actually sends the OK
Mar 18 01:06:59 <denkenz> to give you an idea, check out src/tools/atinterface/ in qt extended
Mar 18 01:07:15 <denkenz> Does exactly the way I propose
Mar 18 01:07:38 <zhenhua> okay, actually i have seen atinterface several times.;-)
Mar 18 01:08:00 <zhenhua> anyway, i can do that.
Mar 18 01:08:38 <denkenz> Again, it is just cleaner for the application, they always call the result function and we figure out when to actually send it
Mar 18 01:08:47 <zhenhua> why not keep the while loop in the main logic? I know buf always true actually
Mar 18 01:09:06 <denkenz> while loop sure, but it should not be while buf 
Mar 18 01:09:15 <zhenhua> while (1)?
Mar 18 01:09:30 <denkenz> how about while (cur < len) or something ;)
Mar 18 01:09:56 <zhenhua> what's the different, boss?
Mar 18 01:10:13 <denkenz> People don't go 'wtf?' when they read the code
Mar 18 01:10:15 <zhenhua> i really think all are good
Mar 18 01:11:33 <zhenhua> i don't have cur variable now. ;-)
Mar 18 01:12:21 <zhenhua> could you have suggestions? ;-)
Mar 18 01:14:21 <denkenz> jeez, do I have to do your job too now? ;)
Mar 18 01:15:01 <zhenhua> ok. so let me think to replace with sth more meaningful
Mar 18 01:15:10 <denkenz> do at least something like while (*buf)
Mar 18 01:15:22 <denkenz> but you can make that loop a heck of alot simpler
Mar 18 01:16:51 <zhenhua> maybe i should wrapper the loop body into a function, right?
Mar 18 01:23:39 <zhenhua> denkenz: any other comments for my patches?
Mar 18 01:36:29 <denkenz> zhenhua: Only other thing is to use switch/case in your at command implementation
Mar 18 01:36:32 <denkenz> will look neater
Mar 18 01:37:38 <zhenhua> i also think about that, but only three cases. and we will have more small chunks of functions.
Mar 18 01:38:07 <zhenhua> i will try that way and send a patch for review
**** ENDING LOGGING AT Thu Mar 18 02:59:56 2010