On 2014-11-15 15:45, Anton Lundin wrote:
On 15 November, 2014 - Jef Driesen wrote:
On 14-11-14 18:01, Anton Lundin wrote:
This is is a RFC series of patches to add support for firmware upgrades on the OSTC3. The code have bin tested both against a "pcb" by HW and against my OSTC3 so it works and does the right things. JeanDo's ostc-companion[1] have bin the reference implementation I've looked at when I wrote this code.
There are still a couple of pain-points that I would like some feedback on:
I still need to have a closer look at all the implementation details. But let's already start with the higher level questions.
How to start the device in service mode: To get access to the necessary commands for service mode, we can't INIT the device with INIT(0xBB), we need to INIT it with a special service mode sequence(0xAA, 0xAB, 0xCD, 0xEF). Now the INIT is sent in hw_ostc3_device_open, so how do you suggest that we refactor the code to allow another sequence after open?
The current quite crude hack is to just EXIT(0xFF) the device and re-init it after a 6s sleep. This won't work against the OSTC Sport.
I'm not really sure what would be the best solution here, so I'm just providing some ideas here:
One solution would be to remove the INIT from open, and maintain the current state (none, download or service) in the device handle. Initially the state would be "none". Then, in each of the public functions, we check the state and if it's "none", we automatically enter download or service mode. If we're in the wrong state, we fail an error.
Another alternative is to have a second variant of the open function that takes a parameter to indicate which mode you want. The tricky part here, is that the high-level dc_device_open() function won't know about this function, so it will always call the download variant. Thus to flash the firmware, an application would be forced to use the new variant. (Note that after v0.5 I intend to remove the vendor_product_device_open() functions from the public api and require applications to use the generic dc_device_open() function instead. But the service mode variant can stay as a vendor specific function, just like the hw_ostc3_device_fwupdate function itself.)
Yet another option is to remove the dc_device_t parameter from the hw_ostc3_device_fwupdate function, and let it open the device itself. Then we call the service mode variant of the open function internally.
On the other hand, I noticed some comment that the key is different for the ostc sport. So that means we somehow have to detect the right model. This is done with the version command, but I'm not sure if you can use this command in service mode. So we may need to identify the exact model in download mode first and then switch to service mode anyway. You say this won't work for the sport, so what's the problem there? Is that maybe because it exits bluetooth mode, and thus the serial port disappears? (I haven't used a sport myself, so I simply don't know.)
Right now, I have a preference towards the second option, with a separate open function for the service mode. It looks the least complex to me. But I'm not ruling out other options yet.
I was more looking at how the api for hw_ostc_device_fwupdate looks and just created a equivalent api.
For me it really doesn't matter, but it makes sense that the both hw_ostc_device_fwupdate and hw_ostc3_device_fwupdate works the same way.
The main difference between option #1 (e.g. remove init from open) and option #2 (e.g. separate open function for service mode) will not be the hw_ostc_device_fwupdate() function itself.
For option #1, the application would do:
dc_device_open(&device, ...); hw_ostc3_device_fwupdate(device, filename); dc_device_close(device);
This is exactly the same sequence as for downloading dives. It's also identical for the firmware upgrade for the ostc2. Except that you should call the corresponding ostc2 function of course.
So the main advantage is that for the application this is all very straightforward. The disadvantage lies in the backend implementation. In the open() function we don't know yet whether we'll need download or service mode, so we'll have to postpone that decision and not initialize any mode at all. Thus this will require some extra complexity in the backend, because in every function we'll need to check whether the correct mode has already been activated or not. Certainly not impossible, but not very elegant either.
For option #2, the application will need to do:
hw_ostc3_device_open_servicemode(&device, ...); hw_ostc3_device_fwupdate(device, filename); dc_device_close(device);
The main advantage here, is that we can immediately enter the correct mode in the open function. The default hw_ostc3_device_open() will start the download mode, and the hw_ostc3_device_open_servicemode() function will start service mode. This keeps things simple in the backend implementation.
The downside is now that the application can no longer use the generic dc_device_open() function. Internally that one will always enter the normal download mode, because that's of course the normal use-case. But on the other hand, is that really a problem? For firmware updating, you need to use a device specific function anyway (hw_ostc3_device_fwupdate). So if you need some device specific code anyway, I assume calling a different open function is not such a big deal either?
Now you basically do:
dc_device_open(&device, context, descriptor, serialport); switch (dc_device_get_type(device)) { case DC_FAMILY_HW_OSTC: hw_ostc_device_fwupdate(device, fileName); break; case DC_FAMILY_HW_OSTC3: hw_ostc3_device_fwupdate(device, fileName); break; default: /* Not supported. */ break; } dc_device_open(device);
And you just need to change the first line too something like this:
if (dc_descriptor_get_type(descriptor) == DC_FAMILY_HW_OSTC3) { hw_ostc3_device_open_servicemode(&device, context, serialport); } else { dc_device_open(&device, context, descriptor, serialport); }
(Or move the open call into the switch. That might even be better, because then you only open a connection for devices that actually support firmware updating!)
That's in a nutshell why I have a preference towards option #2.
The timing between command and data in write_block: The current hw_ostc3_device_write_block is a bit of copy-paste from hw_ostc3_transfer, to be able to send the data buffer to the device, without a additional copy.
I wouldn't worry about an extra data copy. That's inexpensive compared to the actual I/O. But based on the comments, I guess the real problem is something else? Could it be that it seems there is no echo for the WRITE_BLOCK command?
The echo comes after the data block. I've bin playing with sending it as one write too, but for some reason the device doesn't like that and programs nothing.
I think the magic is the serial_sleep (device->port, 2) between the WRITE_BLOCK + address and the data.
Maybe its not that bad, its only one function that has some special properties that can't use hw_ostc3_transfer.
I was surprised to notice that this was different from the other commands, and I was just wondering whether that was on purpose or by accident. If it doesn't work, then there's probably a good reason why it's different :-) We can check with Matthias.
Library triggered closed state: The device reboots when it gets the FLASH_FIRM-command so how can we "not fail" when the application then tries to talk to the device afterwards, or even call hw_ostc3_device_close?
I assume that the problem is that after the FLASH_FIRM command, we don't need to send the EXIT command in hw_ostc3_device_close? Or did you mean something else?
Exactly that. This is just the cousin to the open/service-mode problem.
Indeed. But that's easy to address once we have decided how we're going to implement the open function. If we are in download mode, we send the EXIT command, and in service mode we don't do anything.
BTW, that reminds me of something else. If the firmware update fails halfway for some reason, is there some command we need to send to exit service mode? (I haven't tried the firmware update yet, so I've no idea what happens in such a case.)
There are also a couple of other FIXME's in the code, which I plan on cleaning up, but I thought that I should send out a first RFC patch series to get some additional feedback going.
Any other ideas about how the code looks?
I noticed a large number of sleep and flush calls. Are those really necessary? I have the feeling some of them were just copy and pasted.
Not really copy-pasted, but re-implemented the same ones as where in ostc-companion. I've removed most of them by now, and the ones left there kinda needs to be there.
If they are necessary to get the firmware update to work, then of course they need to stay. But if not, they are only slowing things down unnecessary. Hence my question.
Jef