Rewrite of the stlinkv2 driver.

mjagdis commented 6 years ago

I, like many(?) others, found the previous version somewhat random as to whether it worked or not. I started out by trying to get the debug to say something useful and by the time it did I found I'd pretty much rewritten quite a large part anyway. So once it was working, corner cases and this-can-never-happens dealt with etc. I added support for high speed mode and read-before-write in order to avoid unnecessary erase-and-rewrite cycles. Oh, and did I mention the debug? (Try a build with "make DEBUG=1")

If you use this and still have problems you most likely have power issues (check RST for dips every 100us or so) or incorrect connections (the pin outs on the Chinese ST-Link V2 clones are NOT all the same!)

Signed-off-by: Mike Jagdis mjagdis@eris-associates.co.uk (github: mjagdis)

spth commented 6 years ago

Could you add your position on the possible license change (GPLv2 vs GPLv2 or later) in LICENSE-CHANGE?

I see an

#if defined(__linux__) || defined(__APPLE__) || defined(WIN32)

in the code. Do we support any other platforms? Should this be unconditional?

Philipp

mjagdis commented 6 years ago

As far as I am aware libusb_claim_interface is a standard part of the API, should always be called and libusb will do whatever is necessary for the platform (including nothing). However I don't have the means to test on anything other than linux...

spth commented 6 years ago

I have done tests to check that this does not break existing functionality.

Wrote flash for STM8S105 using integrated ST-Link of STM8S-Discovery
Wrote flash for STM8AF5288 using integrated ST-Link/v2 of STM8A-Discovery
Wrote flash of STM8S103 using original ST-Link/v2
Wrote flash of STM8S103 using clone ST-Link/v2

Philipp

spth commented 6 years ago

I noticed that this changes the behaviour of stm8flash (in the configurations for which I verified that writes to flash work):

Previously, after writing flash, the program would start executing.
In your branch, this does not happen. One can start execution of the program by e.g. power-cycling.

Is this change intentional? Is it surprising to stm8flash users?

Philipp

spth commented 6 years ago

Compiling this with GCC 8 or LLVM 6 shows a few warnings which seem to indicate code quality issues :

stlink.c:62:6: warning: unused variable ‘recv’
stlink.c:398:16: warning: unused variable ‘buf’
stlink.c:484:7: warning: unused variable ‘r’
stlinkv2.c:196:1: warning: no return statement in function returning non-void
stlinkv2.c:327:18: warning: unused variable ‘v2’
stlinkv2.c:419:1: warning: control reaches end of non-void function
stlinkv2.c:213:21: warning: ‘msg_recv_int32’ defined but not used
stlinkv2.c:198:13: warning: ‘msg_recv0’ defined but not used
main.c:127:2: warning: ‘libusb_set_debug’ is deprecated: Use libusb_set_option instead

Could you look into these to check if they are real issues that should be fixed? Also, I think we can safely remove the #if around the libusb_claim_interface call.

Philipp

P.S.: Also, the branch contains a stm8flash binary, which it shouldn't.

mjagdis commented 6 years ago

Are you sure? Main.c calls pgm->reset after a write_range and for stlinkv2 that maps to an srst which should reset the MCU. That should be all that's needed although previously I think stlinkv2 did rather more resetting.

Sent from my iPad

On 22 Aug 2018, at 11:29, Philipp Klaus Krause notifications@github.com wrote:

I noticed that this changes the behaviour of stm8flash:

Previously, after writing flash, the program would start executing. In your branch, this does not happen. One can start execution of the program by e.g. power-cycling. Is this change intentional? Is it surprising to stm8flash users?

Philipp

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

spth commented 6 years ago

I wanted to reproduce the issue of no reset again (since I don't remember if I got it for stlinkv2 only or for both stlinkv2 and stlink). Oddly, it seems something broke in your recent changes:

Determine FLASH area
STLink: v2, JTAG: v0, SWIM: v4, VID: 8304, PID: 4837
Due to its file extension (or lack thereof), "/tmp/test.ihx" is considered as INTEL HEX format!
32079 bytes at 0x380... ^C

with your stm8flash, while

Determine FLASH area
Due to its file extension (or lack thereof), "/tmp/test.ihx" is considered as INTEL HEX format!
207 bytes at 0x8000... OK
Bytes written: 207

With mine.

Philipp

mjagdis commented 6 years ago

Oops! Try this one!

Sent from my iPad

On 23 Aug 2018, at 09:28, Philipp Klaus Krause notifications@github.com wrote:

I wanted to reproduce the issue of no reset again (since I don't remember if I got it for stlinkv2 only or for both stlinkv2 and stlink). Oddly, it seems something broke in your recent changes:

Determine FLASH area STLink: v2, JTAG: v0, SWIM: v4, VID: 8304, PID: 4837 Due to its file extension (or lack thereof), "/tmp/test.ihx" is considered as INTEL HEX format! 32079 bytes at 0x380... ^C with your stm8flash, while

Determine FLASH area Due to its file extension (or lack thereof), "/tmp/test.ihx" is considered as INTEL HEX format! 207 bytes at 0x8000... OK Bytes written: 207 With mine.

Philipp

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

spth commented 6 years ago

Thanks. I had a look and tested using my Debian GNU/Linux system with the integrated ST-Link/v2 of the STM8A-Discovery and the integrated ST-Link of the STM8S-Discovery.

In the branch on the ST-Link the program starts immediately. On the ST-Link/v2 it doesn't. In trunk the program starts immediately for both ST-Link and ST-Link/2.
In the branch, there is still a stm8flash binary, which was eliminated in trunk long ago.
In both the branch and trunk I get the warning that libusb_set_debug() is deprecated and should be replaced by libusb_set_option().

Philipp

mjagdis commented 6 years ago

What do you mean it doesn't start immediately? What does the debug say? (I don't have a Discovery).

The libusb_set_debug deprecation is a separate issue. I've created a separate PR for it but you may decide not to use it for a year or ten. See the comment's on the PR.

spth commented 6 years ago

I just tested again: Write a program onto the STM8AF board of an STM8A-Discovery. I tried with both the current stm8flash master and your fork.

Current stm8flash: Execution of program starts immediately (LEDs blink, etc) Your fork: Program does not start execution (nothing happens). After I power-cycle the board, the program executes (so it apparently got written to the board correctly).

Philipp P.S.: I sent you an email regarding the STM8A-Discovery.

stefaandesmet2003 commented 3 years ago

I like this rewrite, pity it isn't merged yet. the reason why the program doesn't start execution after SRST is because SWIM is still active. After a reset the CPU is immediately stalled again by swim. In the main branch there is this line: stlink2_write_byte(pgm, 0xb6, 0x7f80); it sets the SWIM_CSR.RST bit (not sure why other bits are set though..). Now, after a SRST, SWIM will be off, and CPU running. In your code the close() is not setting this bit, and even then, it should happen before reset()

spth commented 3 years ago

If you like it, I suggest you resolve the conflicts and fix the execution start issue, then make a pull request for the version with the fixes.

stefaandesmet2003 commented 3 years ago

Hi Philipp, sorry for asking : do I fork from the master, or from @mjagdis 's repo? @spth @vdudouyt from your experience, will the SWIM high speed mode & fast block programming in this PR work for all STM8 devices, or is there a risk of breaking support for certain devices?

spth commented 3 years ago

I'm not a git expert (I mostly work on SDCC, which uses svn), but I guess either would work: forking from master, then merging @mjagdis repo or forking from his, then merging what happened in master since; I'd probably try the latter. Either way, when you start making changes in your fork, please state your position on the license change in the file LICENSE-CHANGE.

I don't have much experience with high speed mode. But I wonder if some combinations of ST-Link/v2 clones with some evaluation boards might cause problems. AFAIR, I didn't experience any when testing this PR. but I don't remember how much testing I did back then. This and next week, I'll only have an STM8A-Discovery around for testing, but the week after that, I'll have my full set of STM8 boards (http://www.colecovision.eu/stm8/), and can test various programmer / board combinations then. The current PR has a macro to enable / disable high speed mode, and there is apparently a clear separation of that functionality from the rest. If testing shows that we need slow speed in some situations, we can turn that into a --slow command-line option.

stefaandesmet2003 commented 3 years ago

The merged code is here : https://github.com/stefaandesmet2003/stm8flash/tree/stlink2new Is there a way to link it to this PR #108, or do I have to create a new PR?

There might be an issue with stlink2_high_speed : this function can hang if the SWIM_CSR.HSIT remains 0. I had it once during testing, but couldn't reproduce the exact conditions. is there a reason why the programmer's close() functions are not called from main.c ? @mjagdis added this in his version, but I left it out for now.

spth commented 3 years ago

I'm not that familiar with github, but I guess a new PR is the way to go.

spth commented 3 years ago

Closing this one, since we have an updated version in https://github.com/vdudouyt/stm8flash/pull/144

vdudouyt / stm8flash

Rewrite of the stlinkv2 driver. #108