Set only SCK and SDO pins to 0 after sending command

[falcon35180]

Modified FT245R programmer to only set SCK and SDO pins to 0 after sending the command. This way if e.g. RESET was active before the command, it will stay active. Was having some trouble communicating with an AT89S52, was only able to make it work by manually connecting the reset line to 5V - this fixed the problem.

Also added a #define to allow dumping of raw data sent to the FTDI chip - I used this to help track down the problem.

[stefanrueger]

Thanks for the PR.

this fixed the problem

I have no doubt it did, but what makes us sure that the change doesn't introduce a problem when programming other parts?

Also added a #define to allow dumping of raw data sent to the FTDI chip - I used this to help track down the problem.

I predict the conditional code won't compile: note the missing _ in msginfo()

[falcon35180]

Yes, there was a typo on my part! I had different code originally, then I changed to msg_info and didn't proofread thoroughly, but at least that's an easy fix!

As for the change to ft245r_cmd, I did wonder about that. As I don't have anything else on hand to test with, I would certainly encourage everyone to review my changes and see if I missed something. Please test it as well and see how it fares.

I think there was a potential problem lurking in the original code anyway, since it simply set all the output pins to 0 without regard for any inversions in the programmer configuration.

[stefanrueger]

all the output pins to 0 without regard for any inversions in the programmer configuration

Does the current PR address this?

The AVRDUDE project prefers PR code to be tested and, if possible, some sound arguments why changes wont introduce regressions. The FTDI bitbang driver is some 20 years old (and venerable!). Maybe Theodore @troth or Jeff @jkent want to comment/review on this PR?

[stefanrueger]

I just checked: The AT89S51/2 have active high reset. So, that's a marked difference to all the other AVRs.

[falcon35180]

Does the current PR address this?

I used the SET_BITS_0 macro, which handles that in its expression. Along with the fact that (as far as I could see after careful checking) SCK and SDO are the only bits modified during the clocking out of data bits, I am reasonably confident that the code should work. Happy to be corrected if I'm wrong!

Regarding the use of active high RESET, I worked around that by creating a new programmer definition with the RESET pin inverted.

Anyway, I wanted to contribute my modifications/improvements for everyone's benefit rather than keeping it all to myself. I would welcome any feedback.

[stefanrueger]

contribute [...] for everyone's benefit rather than keeping it all to myself. I would welcome any feedback.

Thanks. Yes, this is great, and we welcome PRs. In absence of any theoretical insight why this PR works for all other relevant parts, in absence of tests with other parts, and in absence of reviews from people who contributed the driver in the first place, I think it would be wise to have a programmer-specific option that only runs the changed code if the user explicitly wants that. This way current behaviour would not change.

You could check out serprog.c in the current git main for how to implement ft245r_parseextendedparms(). Be sure to check current git main as we have just now changed the recommended way of parsing -x options. This also would need documenting in avrdude.1 and doc/avrdude.texi (again, check out how serprog does this).