OSTC3 firmware upgrades

[Anton Lundin]

On 21 November, 2014 - Jef Driesen wrote:

> 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.
> 


I did some hacking earlier this week and implemented #1, because that
was the variant I felt for when i thought about it. I can send those
patches out a bit later if you would like to see.


If you feel strongly for the #2 option, it wouldn't be brain surgery to
rewire it into a #2 variant.


> >>>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.
> 

I think there is some magic timing there, and it probably have $reasons.

You can later the results of my experiments in a #if 0 in a patch I'm
going to send later.

> >>>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.)
> 

In the solution i wrote it will actually enter service mode and only if
it sent FLASH_FIRM (0x50) it will skip the EXIT command, othervise it
will EXIT from service mode the same way as it exits download mode.

> >>>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.
> 

Yea, I've cleaned up a bit of those. My first iteration i sent out was
pretty much a re-implementation of how ostc-companion did it. Now I've
looked at it and how the blocking on read is done in hw_ostc3_transfer
solves most of those problems.


Yesterday I got a Ostc Sport to develop against (Thanks HW!) so i plan
to verify and write some support for the Ostc Sport to.


Would you like to get some "merge-ready" patches for Ostc3 or would you
like to wait before the Ostc Sport patches are ready too?

I would say that it would be good to merge the Ostc3 patches, and then
iterate on-top of that for the Ostc Sport support.


//Anton

-- 
Anton Lundin	+46702-161604