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