OSTC3 firmware upgrades

[Jef Driesen]

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