[PATCH] Cochran_commander refactoring

[Jef Driesen]

John,

I had another look at your latest code today. Armed with your documentation, I 
tried to understand the logic behind it. I have some comments and questions for you:

The split between the config and data is already a nice improvement. There is 
still room for improvement in the sense that the config should be all constant 
and not based on the input data (e.g. the sample_memory_end_address field).

There is a nasty bug in the cochran_commander_serial_setup() function. If there 
is an error, you close the serial port and free the device structure. But this 
function is called from several places, without checking the return value. You 
need to refactor this function to not free anything.

(You also don't need to pass the dc_context_t pointer as a parameter, because 
it's available from the device struct.)

The cochran_commander_serial_open() function is similar. Although it's not 
really a problem here, it's confusing that it frees the device struct. I would 
remove this function, and call serial_open() directly in the 
cochran_commander_device_open() function.


The remainder of this email is mostly some questions and comments on the data 
format an how you handle that in the code:

With the exception of the id, config and misc packets, the protocol is mainly 
based on simple random access to the internal memory. There is no dedicated 
command to read the logbook and samples. They are simply stored in a certain 
area of the internal memory. So for the dump api it makes much more sense to 
implement as full memory dump, instead of reading just two "random" pieces of 
the memory. The result is that the dump would be much more generic, because you 
no longer need to interpret the data (in order to find the logbook and samples). 
This is important when you need the memory dump to diagnose bugs in the logic 
for downloading the dives. The fact that we don't seem to know the total amount 
of memory, might be a problem here.

The next thing where I have some concerns, is the logic how you find the logbook 
and samples. It seems we only know the start address of the logbook and sample 
ringbuffers (respectively 0 and 0x094000 for the emc), but not the end address. 
And that results in a few issues:

For the logbook, you assume the first logbook entry is at the address 0, and 
then move forward until you reach the total number of dives. But in practice, 
the number of dives that can be stored is not unlimited, and the memory is most 
likely used in a circular way. Thus, once you have enough dives, this assumption 
will no longer be correct. Ideally we need to figure out the end of the 
ringbuffer, so we can take this into account correctly. If we are unable to find 
this address, then we need at least some checks to verify our assumptions (for 
example check that the dive number starts at zero and always increases, check 
that we don't go past the sample area, etc). If you ask me, returning an error 
is always better than silently returning some wrong data.

A similar problem exist when reading the samples. You have some code that tries 
to guess where the ringbuffer ends. But there is absolutely no guarantee that 
those assumptions are correct. I think it's much safer to not guess at all and 
just fail if the ringbuffer wrap point is crossed (e.g. begin address is higher 
than the end address). Once someone hits this error, we can investigate and 
determine the correct ringbuffer end address. If we're guessing, and get it 
wrong, then we're silently returning bogus data. That's not good. And the chance 
that your guess is wrong is pretty high. Although I agree that the total memory 
is likely nicely aligned, the sample ringbuffer can end anywhere. I've seen that 
more than once in other backends.

[One method that I have used successfully in the past to locate the ringbuffer 
end, without having access to data that crosses it, is by looking at the full 
memory dump. Usually unused data in flash memory contains all 0xFF. So if there 
is another block located right after the ringbuffer area, then the unused tail 
of the ringbuffer will contain 0xFF, followed by some non 0xFF data from that 
other block. Thus the first address containing non 0xFF bytes is very likely the 
end of the ringbuffer.]

The next big question is where does the sample data begins/ends? So far you have 
identified three pointers: pre, begin and end address. When I dump those 
pointers (see below), I noticed something interesting:

  * The 5th column is the difference between begin and pre. According to your 
documentation, these are inter dive events. They can be zero length too. Should 
we consider those as part of the dive, or ignore them? Since I don't know what's 
in those interdive events, I have no idea.

  * The 6th column is the difference between pre and end of the previous entry. 
This is nearly always equal to 1800. I doubt that's a coincidence. I guess that 
means there is some 1800 byte header or trailer. Do you have any idea what's in 
there?

* When the end pointer is ffffffff, the next entry has the same pre pointer. 
Because it makes no sense that the sample data for two different dives start at 
the same address, I assume this means that for some reason (e.g. battery empty, 
too short duration, etc) this dive has no sample data. What you did in this case 
is take the data until the start of the next dive. But if I'm right, that's the 
inter dive events. So this makes no sense to me. Do you get anything useful from 
doing this?


I also tried to parse the test data I received from you, but I can't get 
anything useful out of it. That might be due to the fact that I don't know at 
which address the sample data was read when you created that data (that might 
have changed in your latest code). Btw, that's exactly why I prefer a more 
generic dump format as explained above. Can you provide some new data for me, or 
the correct address? I also tried to run the parser under valgrind, and it 
reports a *huge* number of problems. That might be due to this mismatch in data, 
but the parser should be robust against whatever you throw at it. The rule of 
thumb is never trust the input data, and always check everything before you use 
it. I didn't investigate further, because I first want to confirm with good 
input :-)


Jef

  i |   pre  | begin  |  end  | 5th | 6th
==========================================
   0 00094000 00094834 00094a95 2100 606208
   1 0009519d 0009519d 0009674b 0 1800
   2 00096e53 00096e7d 000980c9 42 1800
   3 000987d1 00098aa9 ffffffff 728 1800
   4 000987d1 00098e3c 0009ac28 1643 624594
   5 0009b330 0009b330 ffffffff 0 1800
   6 0009b330 0009b43c 0009c2c4 268 635697
   7 0009c9cc 0009ca46 0009e898 122 1800
   8 0009efa0 0009f01d 000a07f4 125 1800
   9 000a0efc 000a0f39 000a2d60 61 1800
  10 000a3468 000a34a6 000a4f2a 62 1800
  11 000a5632 000a5647 000a7207 21 1800
  12 000a790f 000a7a3c ffffffff 301 1800
  13 000a790f 000a8e3c ffffffff 5421 686352
  14 000a790f 000a983c 000aa9e6 7981 686352
  15 000ab0ee 000ab26c 000ab40a 382 1800
  16 000abb12 000abb12 000abdc5 0 1800
  17 000ac4cd 000ac4cd 000ad6c9 0 1800
  18 000addd1 000ade4c 000ae5d3 123 1800
  19 000aecdb 000aecdb 000b06cd 0 1800
  20 000b0dd5 000b0e69 000b40aa 148 1800
  21 000b47b2 000b481e ffffffff 108 1800
  22 000b47b2 000b5466 000b70c3 3252 739251
  23 000b77cb 000b77df 000b9d11 20 1800
  24 000ba419 000ba469 000bda64 80 1800
  25 000be16c 000be217 000c00ac 171 1800
  26 000c07b4 000c084a 000c29ed 150 1800
  27 000c30f5 000c34d6 000c4a9f 993 1800
  28 000c51a7 000c51a7 000c6449 0 1800
  29 000c6b51 000c6e74 000c6e7d 803 1800
  30 000c7585 000c79ff 000c9d39 1146 1800
  31 000ca441 000ca47f 000cc82d 62 1800
  32 000ccf35 000ccf73 000ce838 62 1800
  33 000cef40 000cefa7 000d15c7 103 1800
  34 000d1ccf 000d1cf8 000d47ed 41 1800
  35 000d4ef5 000d4f09 000d77c4 20 1800
  36 000d7ecc 000d7ee0 000da532 20 1800
  37 000dac3a 000dac4e 000dd172 20 1800
  38 000dd87a 000dd88e 000df0bd 20 1800
  39 000df7c5 000df7ee 000e2f19 41 1800
  40 000e3621 000e364a 000e5730 41 1800
  41 000e5e38 000e5e4c 000e880c 20 1800
  42 000e8f14 000e8f28 000eb7cf 20 1800
  43 000ebed7 000ebeeb 000eee22 20 1800
  44 000ef52f 000ef543 000f1e4f 20 1805
  45 000f255b 000f2725 000f2761 458 1804
  46 000f2e69 000f2e69 000f599f 0 1800
  47 000f60a7 000f60f9 000f8e7f 82 1800
  48 000f9587 000f95b0 000fb7e2 41 1800
  49 000fbeea 000fbfd8 000febe4 238 1800
  50 000ff2ec 000ff397 00100e44 171 1800
  51 0010154c 0010154c 0010160c 0 1800
  52 00101d14 00101dd9 00103a7e 197 1800
  53 00104186 001041f2 00105a4a 108 1800
  54 00106152 00106167 00107b79 21 1800
  55 00108281 001083d0 0010b8cc 335 1800
  56 0010bfd4 0010c012 0010f0dc 62 1800
  57 0010f7e4 0010f7f8 00111a89 20 1800
  58 00112191 001121cf 00114e23 62 1800
  59 0011552b 0011553f 00118412 20 1800
  60 00118b1a 00118b58 0011b9a5 62 1800
  61 0011c0ad 0011c0c1 0011e8ab 20 1800
  62 0011efb3 0011efdc 00121e25 41 1800
  63 0012252d 00122541 00124dc4 20 1800
  64 001254cc 001254e0 00128055 20 1800
  65 0012875d 00128771 0012b036 20 1800
  66 0012b73e 0012b767 0012e4ba 41 1800
  67 0012ebc2 0012ebd6 00130d22 20 1800
  68 0013142a 0013143e 00133d2f 20 1800
  69 00134437 00134437 00136ef7 0 1800
  70 001375ff 00137628 0013912e 41 1800
  71 00139836 0013984a 0013c3f2 20 1800
  72 0013cafa 0013cdbc 00140ab6 706 1800
  73 001411be 00141269 00143321 171 1800
  74 00143a29 00143a29 001455fa 0 1800
  75 00145d02 00145d42 00148ae2 64 1800
  76 001491ea 0014933f 0014a805 341 1800
  77 0014af0d 0014af0d 0014b1eb 0 1800
  78 0014b8f3 0014b94a 0014ef9d 87 1800
  79 0014f6a5 0014f70d ffffffff 104 1800
  80 0014f6a5 0015043c 00151442 3479 1373862
  81 00151b4a 00151b4a 00153815 0 1800
  82 00153f1d 00153fb3 ffffffff 150 1800
  83 00153f1d 0015643c ffffffff 9503 1392414
  84 00153f1d 0015663c ffffffff 10015 1392414
  85 00153f1d 001566e0 ffffffff 10179 1392414
  86 00153f1d 0015943c ffffffff 21791 1392414
  87 00153f1d 0015983c 0015a55f 22815 1392414
  88 0015ac67 0015ad0e ffffffff 167 1800
  89 0015ac67 0015ca3c 0015d1c0 7637 1420392
  90 0015d8c8 0015d8f1 0015fa0c 41 1800
  91 00160114 00160129 ffffffff 21 1800
  92 00160114 0016043c ffffffff 808 1442069
  93 00160114 00161c3c 001620bf 6952 1442069
  94 001627c7 001627db ffffffff 20 1800
  95 001627c7 0016447b ffffffff 7348 1451976
  96 001627c7 0016463c 001661b3 7797 1451976
  97 001668bb 0016694d 00167a66 146 1800
  98 0016816e 0016853f 00169fa2 977 1800
  99 0016a6aa 0016a6aa 0016c07d 0 1800
100 0016c785 0016cae1 0016f7b4 860 1800
101 0016febc 0016fee5 00172dee 41 1800
102 001734f6 0017350a 00175cdb 20 1800
103 001763e3 0017640c 00177487 41 1800
104 00177b8f 00177bd1 00179239 66 1800
105 00179941 00179955 0017aae1 20 1800
106 0017b1e9 0017b212 0017c680 41 1800
107 0017cd88 0017ce2f 0017fbce 167 1800
108 001802d6 001802ff 00182cfe 41 1800
109 00183406 0018342f 0018591f 41 1800
110 00186027 00186066 0018821d 63 1800
111 00188925 001889a2 0018b8a6 125 1800
112 0018bfae 0018c452 0018d833 1188 1800
113 0018df3b 0018df8e 00190d5f 83 1800