My first patch
Dirk Hohndel
dirk at hohndel.org
Sat Sep 5 08:55:32 PDT 2015
Hi Guido,
patches should go to the mailing list so more people can comment and
review.
I will try to comment on everything that I notice in here.
The first comment, of course, is that this should NOT have been just one
commit. There should have been many small commits that each do one thing.
For example there are a few white space changes where you aligned comment
without changing the code. Those should be in a separate commit.
You changed the meaning of the DEBUG bits (nothing wrong with that - they
were a complete mess, anyway). But again, that should be a separate
commit.
On Sat, Sep 05, 2015 at 04:59:27PM +0200, Guido Lerch wrote:
>
> Design change to support my 200+ dives that are very long, previous version could not handle this.
The commit title should concisely describe the change and be no more than
72 characters. This is what people see in a git log. So a good commit
title would be something like
Uemis downloader: design change to better deal with many long dives
This way it's clear which area this works on (Subsurface happily supports
200+ very long dives from sane dive computers... it's only the Uemis where
this previously was a problem).
> - changed the way on how dive logs are mapped to dive details in (do_uemis_import)
> - dives deleted on the Uemis will not be downloaded anymore (added function uemis_delete_dive_by_diveid)
> - changed the way the memory consumption was checked to acknowledge very large files (trying to predic on how many more files fit into
> the buffer by calculating an average consumbtion over each divelogs block)
> - minimal design change to support the above
The body of the commit message should be no longer than 72 characters per
line as well. Sometimes that's not possible, but it's something to strive
for.
> Signed-off-by: Guido Lerch <glerch at Guido-Office.local>
Do you want to set a valid email address for signing off commits?
The easiest way to do that is
git config --global user.email "guido.lerch at gmail.com"
> diff --git a/uemis-downloader.c b/uemis-downloader.c
> index baf9484154e2..a6f71f5a7fee 100644
> --- a/uemis-downloader.c
> +++ b/uemis-downloader.c
> @@ -9,6 +9,15 @@
> * I believe that I only used the information about HOW to do this (download data from the Uemis
> * Zurich) but did not actually use any of his copyrighted code, therefore the license under which
> * he released his code does not apply to this new implementation in C
> + *
> + * Modified by Guido Lerch guido.lerch at gmx.ch in August 2015
> + * Slight design changes to achieve a more reliable mapping between divelogs and dive details.
> + * On my UEMIS I had deleted several dives and I had some very long dives which cause the old design
> + * to fail an not mapping any dive spots and wrong divelog to divedetail mapping. Also the offset calculations
> + * caused with my UEMIS the dives 12 and 13 to be read over and over causing an endless loop till the UEMIS
> + * memory was full.
> + * end August changes ( 31 August 2015 )
> + * I will look at the buddies next.
This is more something that would go into the commit log.
When looking at the source file it's not really all that interesting what
was broken before or what you are going to read on next.
It is of course OK to add your copyright here
> */
> #include <fcntl.h>
> #include <dirent.h>
> @@ -28,11 +37,23 @@
> #define BUFLEN 2048
> #define NUM_PARAM_BUFS 10
>
> -#if UEMIS_DEBUG & 64 /* we are reading from a copy of the filesystem, not the device - no need to wait */
> -#define UEMIS_TIMEOUT 50 /* 50ns */
> -#define UEMIS_LONG_TIMEOUT 500 /* 500ns */
> -#define UEMIS_MAX_TIMEOUT 2000 /* 2ms */
> +/*
> + * glerch Aug. 2015
Don't mark comments like this - git blame will tell me if I want to know
who wrote a comment and when
> + * cheating as I dont know how to set this in QT :-(
> + */
> +#define UEMIS_DEBUG 1+2
Well, when you send me a patch /usually/ it's a good idea to have debug
output disabled... simply comment out this line if you want to keep it
there for reference
> +#define UEMIS_MAX_FILES 4000.0
Why is that a floating point number? FP comparisons are notoriously
tricky...
> +#define UEMIS_MEM_FULL 3
> +#define UEMIS_MEM_CRITICAL 1
> +#define UEMIS_MEM_OK 0
> +
> +#if UEMIS_DEBUG & 64 /* we are reading from a copy of the filesystem, not the device - no need to wait */
> +#define UEMIS_TIMEOUT 50 /* 50ns */
> +#define UEMIS_LONG_TIMEOUT 500 /* 500ns */
> +#define UEMIS_MAX_TIMEOUT 2000 /* 2ms */
> #else
> +
As mentioned earlier - having the white space change in the middle of
other changes makes things harder to read... better to have that in a
separate commit
> #define UEMIS_TIMEOUT 50000 /* 50ms */
> #define UEMIS_LONG_TIMEOUT 500000 /* 500ms */
> #define UEMIS_MAX_TIMEOUT 2000000 /* 2s */
> @@ -49,9 +70,17 @@ static int filenr;
> static int number_of_files;
> static char *mbuf = NULL;
> static int mbuf_size = 0;
> -
> static int nr_divespots = -1;
>
> +/* glerch Aug 2015
see above, please don't mark comments
> + * adding to avoid re-reads of the dive spots if multiple UEMIS re-connects are needed
> + */
> +// static int iDiveSpotStart = 0;
> +static int iBuddiesStart = 0;
> +static int iBuddies = -1;
> +static int iMaxMemUsed = -1;
> +static int iNextTableIndex = 0;
I know this is silly and I know we aren't consistent, but we are aiming
for two different naming styles of our variables...
C code in general has underscores and lower case
C++ code uses camelCase
the idea is that this will tell you where a variable originates
We aren't as consistent as I would like, but especially when editing an
existing file it is good practice to follow the same style.
And what's that 'i' prefix on all those names? Please don't tell me that
this indicates that they are integers...
> +
> /* helper function to parse the Uemis data structures */
> static void uemis_ts(char *buffer, void *_when)
> {
> @@ -114,13 +143,22 @@ static void uemis_get_weight(char *buffer, weightsystem_t *weight, int diveid)
>
> static struct dive *uemis_start_dive(uint32_t deviceid)
> {
> - struct dive *dive = alloc_dive();
> + struct dive *dive = alloc_dive();
White space. We indent with tab, not spaces
>
> +/* glerch Aug 2015 */
> +static struct dive *uemis_get_dive_by_uemis_diveid(device_data_t *devdata, u_int32_t object_id) {
> + for (int i = 0; i < devdata->download_table->nr; i++) {
> + if (object_id == devdata->download_table->dives[i]->dc.diveid)
> + return devdata->download_table->dives[i];
> + }
> + return NULL;
> +}
Whites space. Indent with tab
> @@ -372,7 +410,7 @@ static char *first_object_id_val(char *buf)
> char *p = object + 14;
> char *t = tmp;
>
> -#if UEMIS_DEBUG & 2
> +#if UEMIS_DEBUG & 4
> char debugbuf[50];
> strncpy(debugbuf, object, 49);
> debugbuf[49] = '\0';
> @@ -408,8 +446,8 @@ static void show_progress(char *buf, const char *what)
> char *val = first_object_id_val(buf);
> if (val) {
> /* let the user know what we are working on */
> -#if UEMIS_DEBUG & 2
> - fprintf(stderr, "reading %s %s %s\n", what, val, buf);
> +#if UEMIS_DEBUG & 16
> + fprintf(debugfile, "reading %s\n %s\ %s\n", what, val, buf);
more whitespace issues - and as I said, this should definitely have been a
separate commit
> @@ -440,12 +478,12 @@ static bool uemis_get_answer(const char *path, char *request, int n_param_in,
> bool answer_in_mbuf = false;
> char *ans_path;
> int ans_file;
> - int timeout = UEMIS_LONG_TIMEOUT;
> + int timeout = UEMIS_LONG_TIMEOUT;
whitespace - I'll stop commenting on that now... there are lots and lots
of issues all over the file.
> @@ -467,7 +505,7 @@ static bool uemis_get_answer(const char *path, char *request, int n_param_in,
> file_length = strlen(sb);
> snprintf(fl, 10, "%08d", file_length - 13);
> memcpy(sb + 5, fl, strlen(fl));
> -#if UEMIS_DEBUG & 1
> +#if UEMIS_DEBUG & 4
> fprintf(debugfile, "::w req.txt \"%s\"\n", sb);
> #endif
> if (write(reqtxt_file, sb, strlen(sb)) != strlen(sb)) {
> @@ -479,13 +517,13 @@ static bool uemis_get_answer(const char *path, char *request, int n_param_in,
> more_files = false;
> }
> trigger_response(reqtxt_file, "n", filenr, file_length);
> - usleep(timeout);
> + usleep(timeout);
> mbuf = NULL;
> mbuf_size = 0;
> while (searching || assembling_mbuf) {
> if (import_thread_cancelled)
> return false;
> - progress_bar_fraction = filenr / 4000.0;
> + progress_bar_fraction = filenr / UEMIS_MAX_FILES;
Yes, here you want the fp - so simply cast it (double)UEMIS_MAX_FILES
> +static bool uemis_delete_dive(device_data_t *devdata, uint32_t diveid) {
> + struct dive *dive = NULL;
> +
> + if (devdata->download_table->dives[devdata->download_table->nr - 1]->dc.diveid == diveid)
why aren't you using the helper function to get that dive?
> + /* we hit the last one in the array */
> + dive = devdata->download_table->dives[devdata->download_table->nr - 1];
ditto
> + else {
this is REALLY ugly. I think this is covered in the CodingStyle; if one
half of an if clause needs curly braces, have them on both sides
if (cond) {
one line;
} else {
multi;
line;
}
> + for(int i= 0; i < devdata->download_table->nr - 1; i++) {
more whitespace: for (int i = 0; ...
> + if (devdata->download_table->dives[i]->dc.diveid == diveid ) {
again, use the helper to make this easier to read
> @@ -711,6 +798,10 @@ static void parse_tag(struct dive *dive, char *tag, char *val)
> * index into yet another data store that we read out later. In order to
> * correctly populate the location and gps data from that we need to remember
> * the adresses of those fields for every dive that references the divespot. */
> +
> +/* glerch Aug 2015
> + * I start using the for_dive variable to actually identify via the object_id
> + */
I wouldn't make first person comments when describing code functionality
> + /* glerch Sep. 2015
> + * maybe I am missing something here but this seems wrong
Yes, this is a first person comment :-)
> if (keep_number)
> dive->number = atoi(val);
> - } else if (!log && !strcmp(tag, "logfilenr")) {
> + */
> + } else if (isDive && strcmp(tag, "logfilenr") == 0) {
> /* this one tells us which dive we are adding data to */
> - dive = get_dive_by_uemis_diveid(atoi(val), deviceid);
> + dive = uemis_get_dive_by_uemis_diveid(devdata, atoi(val));
all these whitespace changes make things really hard to read
> if (for_dive)
> *for_dive = atoi(val);
> - } else if (!log && dive && !strcmp(tag, "divespot_id")) {
> - dive->dive_site_uuid = create_dive_site("from Uemis", dive->when);
> + } else if (!isLog && dive && !strcmp(tag, "divespot_id")) {
> + timestamp_t t;
> + dive->dive_site_uuid = create_dive_site("from Uemis", (int)time(NULL));
So you have a timestamp_t t here, but then call (int)time(NULL)... why?
and what's the purpose. you have a dive, whiy not use the dive's time as
the code you replaced did?
> +static int bufCnt = 0;
> +static bool do_dump_buffer_to_file(char *buf, char* prefix, int round) {
> + char path[100];
> + char date[40];
> + char obid[40];
> + if (!buf)
> + return false;
> +
> + if (strstr(buf, "date{ts{"))
> + strlcpy(date, strstr(buf, "date{ts{"), sizeof(date));
> + else
> + strlcpy(date, strdup("date{ts{no-date{"), sizeof(date));
> +
> + if (!strstr(buf, "object_id{int{"))
> + return false;
> +
> + strlcpy(obid, strstr(buf, "object_id{int{"), sizeof(obid));
> + char *ptr1 = strstr(date, "date{ts{");
> + char *ptr2 = strstr(obid, "object_id{int{");
> + char *pdate = next_token(&ptr1); pdate = next_token(&ptr1); pdate = next_token(&ptr1);
> + char *pobid = next_token(&ptr2); pobid = next_token(&ptr2); pobid = next_token(&ptr2);
> + snprintf(path, sizeof(path), "/Users/glerch/UEMIS Dump/%s_%s_Uemis_dump_%s_in_round_%d_%d.txt",prefix, pdate, pobid, round, bufCnt);
Hmm - that's maybe not ideal... :-)
> +/* glerch Aug 2015
> + * do some more sophisticated calculations here to try and predict if the next round of
> + * divelog/divedetail reads will fit into the UEMIS buffer,
> + * filenr holds now the uemis filenr after having read several logs including the dive details,
> + * fCapacity will five us the average number of files needed for all currently loaded data
> + * remember the maximum file usage per dive
> + * return : 0 if there is enough memeory for a full round
> + * 1 if the memory is good for reading the dive logs
> + * 3 if the memory is exhaused
What's wrong with 2? And didn't you do #defines for that all the way at
the top? Why not use them here?
> +static int get_memory(struct dive_table *td) {
> +
> + if (td->nr == 0)
> + return UEMIS_MEM_OK;
> +
> + if (filenr/td->nr > iMaxMemUsed)
> + iMaxMemUsed = filenr/td->nr;
> + /* predict based on the iMaxMemUsed value if the set of next 11 divelogs plus details
> + * fit into the memory before we have to disconnect the UEMIS and continuem. To be on
> + * the safe side we calculate using 12 dives.
> + al */
> + if ( iMaxMemUsed * 11 > UEMIS_MAX_FILES - filenr) {
And here is where the floating point might cause you pain. I'd suggest an int
> + /* the next set of divelogs will most likely not fit into the memory
> + */
> + if ((nr_divespots /*- iDiveSpotStart*/) * 2 > UEMIS_MAX_FILES - filenr) {
Umm, no. No comments in the middle of formulas
> + /* if we get here we have a severe issue as the divespots will not fit into
> + * this run either.
> + */
> + return UEMIS_MEM_FULL;
> + }
> + /* we continue reading the divespots */
> + return UEMIS_MEM_CRITICAL;
> + }
> + return UEMIS_MEM_OK;
See here you use the constants
> +}
> +
> +/* glerch Sep 2015
> + * mark a dive as deleted by setting download to false
> + * this will be picked up by some cleaning statement later
> + */
> +static void do_delete_dives(struct dive_table *td, int idx)
> +{
> + for (int x = idx; x < td->nr; x++)
> + td->dives[x]->downloaded = false;
> +}
> +
Yikes - now it gets really hard to review things because all of the white
space is hiding what you are actually doing.
> const char *do_uemis_import(device_data_t *data)
> {
> - const char *mountpath = data->devname;
> - short force_download = data->force_download;
> - char *newmax = NULL;
> - int first, start, end = -2;
> - int i, offset = 0;
> - uint32_t deviceidnr;
> - char objectid[10];
> - char *deviceid = NULL;
> - const char *result = NULL;
> - char *endptr;
> - bool success, keep_number = false, once = true;
> -
> - if (dive_table.nr == 0)
> - keep_number = true;
> - uemis_info(translate("gettextFromC", "Initialise communication"));
> - if (!uemis_init(mountpath)) {
> - free(reqtxt_path);
> - return translate("gettextFromC", "Uemis init failed");
> - }
> - if (!uemis_get_answer(mountpath, "getDeviceId", 0, 1, &result))
> - goto bail;
> - deviceid = strdup(param_buff[0]);
> - deviceidnr = atoi(deviceid);
> - /* the answer from the DeviceId call becomes the input parameter for getDeviceData */
> - if (!uemis_get_answer(mountpath, "getDeviceData", 1, 0, &result))
> - goto bail;
> - /* param_buff[0] is still valid */
> - if (!uemis_get_answer(mountpath, "initSession", 1, 6, &result))
> - goto bail;
> - uemis_info(translate("gettextFromC", "Start download"));
> - if (!uemis_get_answer(mountpath, "processSync", 0, 2, &result))
> - goto bail;
> - /* before starting the long download, check if user pressed cancel */
> - if (import_thread_cancelled)
> - goto bail;
> - param_buff[1] = "notempty";
> - /* if we force it we start downloading from the first dive on the Uemis;
> - * otherwise check which was the last dive downloaded */
> - if (!force_download)
> - newmax = uemis_get_divenr(deviceid);
> - else
> - newmax = strdup("0");
> - first = start = atoi(newmax);
> - for (;;) {
> + const char *mountpath = data->devname;
> + short force_download = data->force_download;
> + char *newmax = NULL;
> + int first, start, end = -2;
> + int i = 0;
> + uint32_t deviceidnr;
> + //char objectid[10];
> + char *deviceid = NULL;
> + const char *result = NULL;
> + char *endptr;
> + bool success, keep_number = false, once = true;
So these are all unchanged, just messed up whitespace
> + char divetoRead[10];
> + char logFileNoToFind[20];
> + int deletedFiles = 0;
> + int lastFoundLogfilenr = 0;
> + int iMatchDiveAndLog = 0;
> + int iStartCleanup = 0;
> + struct dive_table *td = NULL;
> + struct dive *dive = NULL;
> + int iUemisMemStat = UEMIS_MEM_OK;
these are new. What's with the 'i' names...
> +
> +#if UEMIS_DEBUG
> + const char *dTime;
> +#endif
> +
> + if (dive_table.nr == 0)
> + keep_number = true;
> +
> + uemis_info(translate("gettextFromC", "Initialise communication"));
> + if (!uemis_init(mountpath)) {
> + free(reqtxt_path);
> + return translate("gettextFromC", "Uemis init failed");
> + }
> +
> + if (!uemis_get_answer(mountpath, "getDeviceId", 0, 1, &result))
> + goto bail;
> + deviceid = strdup(param_buff[0]);
> + deviceidnr = atoi(deviceid);
> +
> + /* param_buff[0] is still valid */
> + if (!uemis_get_answer(mountpath, "initSession", 1, 6, &result))
> + goto bail;
> +
> + uemis_info(translate("gettextFromC", "Start download"));
> + if (!uemis_get_answer(mountpath, "processSync", 0, 2, &result))
> + goto bail;
That's all unchanged? ARGGGGGG
Sorry, I gave up here. I cannot see what's old and what's new and what's
changed.
I can try and clean up the whitespace after your patch was applied and try
again to look at the algorithmic changes. And code changes.
So I'm not sure what to suggest here. Given that you are new to git I
don't think it makes sense to ask you to turn this into multiple commits
to make it easier...
I'm running out of time this morning (it's a holiday weekend, family is
asking for me to join them). I will try to clean this up and turn this
into a commit series, but that will take me a while.
It would have been much much much much much much easier if you had sent me
patches as you went along and wrote this and worked on this.
Oh well. Water under the bridge.
/D
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Design-change-to-support-my-200-dives-that-are-very-.patch
Type: application/octet-stream
Size: 36489 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150905/d7ac471c/attachment-0001.obj>
More information about the subsurface
mailing list