<div dir="ltr">Good morning Dirk.<br><div><div class="gmail_extra"><br><div class="gmail_quote">2015-04-02 18:18 GMT+02:00 Dirk Hohndel <span dir="ltr"><<a href="mailto:dirk@hohndel.org" target="_blank">dirk@hohndel.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> + dc_datetime_t datetime = { 0 };<br>
<br>
so you renamed dt to datetime - nothing wrong with that, but it makes the<br>
patch more verbose and harder to see what's new, what's just renaming...<br>
<br>
> - rc = dc_parser_get_datetime(parser, &dt);<br>
> + rc = dc_parser_get_datetime(parser, &datetime);<br>
<br>
like this.<br>
<br></blockquote><div><br></div><div>It wasn't my finest hour, sure. I'll put it back to dt.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {<br>
> dev_info(devdata, translate("gettextFromC", "Error parsing the datetime"));<br>
> - goto error_exit;<br>
> + fprintf(stderr, "Error parsing the datetime");<br>
> + return rc;<br>
<br>
fprintf to stderr is reasonable for debugging or for fatal errors,<br>
otherwise it's not ideal. Are these errors that should be reported to the<br>
user? Or really just debugging messages?<br></blockquote><div><br></div><div>Just debugging messages, the returned code is managed and shown to the user in the error bar on mainwindow. <br></div><div>I'll get rid of all of them.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> - dive->dc.duration.seconds = divetime;<br>
> + dive->duration.seconds = dive->dc.duration.seconds = divetime;<br>
<br>
Why? fixup_dive() should be called on any imported dive at some point and<br>
that calls fixup_duration() which handles this consistently.<br></blockquote><div><br></div><div>Sorry, this is a remanent from the work in smartrak files, which, for some reason are not correctly managed.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> @@ -537,7 +519,7 @@ static int dive_cb(const unsigned char *data, unsigned int size,<br>
> for (idx = 0; idx < 100; idx++) {<br>
> dc_field_string_t str = { NULL };<br>
> rc = dc_parser_get_field(parser, DC_FIELD_STRING, idx, &str);<br>
> - if (rc != DC_STATUS_SUCCESS)<br>
> + if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED)<br>
<br>
Can you explain this change?<br></blockquote><div><br></div><div>Ufff. This is a bold failure on my side. Too much copy paste here. Didn't even realized it, as OSTC fully supports DC_FIELD_STRING. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> @@ -558,21 +541,67 @@ static int dive_cb(const unsigned char *data, unsigned int size,<br>
> dive->dc.divemode = FREEDIVE;<br>
> break;<br>
> case DC_DIVEMODE_GAUGE:<br>
> - case DC_DIVEMODE_OC: /* Open circuit */<br>
> + case DC_DIVEMODE_OC:<br>
> dive->dc.divemode = OC;<br>
> break;<br>
> - case DC_DIVEMODE_CC: /* Closed circuit */<br>
> + case DC_DIVEMODE_CC:<br>
<br>
And why would you remove those comments?<br>
<br></blockquote><div>Absolutely unintended, the general idea was just splitting the initial callback func in two with the minimal changes.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> #endif<br>
><br>
> - rc = parse_gasmixes(devdata, dive, parser, ngases, data);<br>
> - if (rc != DC_STATUS_SUCCESS) {<br>
> + rc = parse_gasmixes(devdata, dive, parser, ngases, NULL);<br>
> + if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {<br>
<br>
Maybe I got lost reading the patch - but why are you passing NULL instead<br>
of data?<br></blockquote><div><br></div><div>The data pointer is passed to the func but not used. It would even be safely removed. Just didn't it as I wasn't sure if it was intended to be used in the future. I'll add a patch to remove it.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> dev_info(devdata, translate("gettextFromC", "Error parsing the gas mix"));<br>
> + fprintf(stderr, "Error parsing the gas mix\n");<br>
<br>
Umm - so there is the correct error reporting to the user, and you add an<br>
fprintf to stderr. Not good.<br>
<br>
<br>
Here's what I'd like to see. Either don't do the datetime rename or if you<br>
do it, have it in its own patch with nothing else mixed in.<br>
Then, if you want to change all the error returns to show these errors to<br>
the user, do that in one patch and nothing else.<br>
As a third patch you can do the split out into a separate function,<br>
explaining any logic changes that you are doing.<br></blockquote><div><br></div><div>OK. I'll rework the patches removing the debug messages , the datetime change and all other issues.<br><br></div><div>Thanks Dirk.<br><br></div><div>More comments in the other mail.<br><br></div><div>Regards.<br><br></div><div>Salva. <br></div><div><br></div></div></div></div></div>