Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TPI memory writes for odd number of bytes #825

Closed
wants to merge 1 commit into from

Conversation

mariusgreuel
Copy link
Contributor

@mariusgreuel mariusgreuel commented Jan 10, 2022

Kinda a quick shot, review please ;-)

@MCUdude
Copy link
Collaborator

MCUdude commented Jan 10, 2022

Wow, that was quick! I can confirm that this PR fixes issue #823. I'll assume this will only affect AVRs with TPI, and not others? AFAIK all parts with TPI has only one fuse.

Copy link
Contributor

@dl8dtl dl8dtl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work for me.
In order to avoid the segfault, I still need to check for m->op[…] being non-NULL in fuse_get_mask(). (I'll commit that change anyway.)
However, it causes the fuse to be not written in the end.
OTOH, writing the fuse in terminal mode works flawlessly … Will have to investigate further.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jan 11, 2022

I'd like to drop that PR. Fuse writes are now handled differently, and for flash writes, it's IMHO fine to just add another byte to write in order to ensure full 16-bit words are written.

@mariusgreuel
Copy link
Contributor Author

Doesn't work for me. In order to avoid the segfault,

Correct. I removed the "Fixes #" text.

I'd like to drop that PR

There is still a bug in the code, so I think we should merge it. For odd sized buffers, its reading one byte past the buffer and writing a random byte to the devices memory. Also, it returns +1 for the bytes written.

to ensure full 16-bit words are written

I do not know the TPI protocol. If it is a requirement to always write two bytes, I think we should write 0xFF for the second byte.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jan 12, 2022

If it is a requirement to always write two bytes, I think we should write 0xFF for the second byte.

Makes sense to me.
Yes, it's a requirement: the flash organization of an AVR is in units of 16 bits.

@MCUdude
Copy link
Collaborator

MCUdude commented Jan 12, 2022

This PR does seem to work in combination with the recently merged #828. Both with -Ufuse:w:0xfd:m and in terminal mode

@mariusgreuel
Copy link
Contributor Author

I looked around a little, and the entire file needs some fixing - all page and word based read/write functions appear to be buggy. #784 belongs to the same category.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jan 12, 2022

OK, then it needs a bit more of work.
Odd-count writes to flash are most likely not much of a problem, as the AVR flash is always used in 16-bit words anyway (each instruction is 16 bits or a multiple of that, and data are usually padded to two-byte boundaries).

@mcuee
Copy link
Collaborator

mcuee commented Aug 24, 2022

#1068 was deleted by the poster but it has raised similar question about odd number of bytes.

AVRDUDE fails with writing to FLASH if there are even number of bytes. If bytes count is odd, everything is fine.
I suppose it skips writing for single byte, because AVR commands are always 2(or sometimes, 4) bytes length.
System - Win10_x64
MCU - ATmega8535
Programmer - PonyProg like with COM(RS232) interface.

Actually 1st time I faced this bug in old 2007, with LPT based programmer. I've workaround it by manual add 0xFF to binary file. So, I am surprized, when I saw same problem in 2022. It's weird, that no one reported it before.
Issue is reproduced with bin and with hex file format. And I'm sure it is not programmer or MCU dependent.

@mcuee
Copy link
Collaborator

mcuee commented Aug 24, 2022

@dl8dtl's reply

The AVR flash is organized in units of 16 bits. So you cannot write a single 8-bit entity to it.

Strange enough, the LPM instruction can read single bytes out of it anyway. (The SPM instruction can only write 16-bit words.)

Normally, any program that produces flash data for the AVR is supposed to pad them to a full 16-bit word.

@mcuee
Copy link
Collaborator

mcuee commented Aug 24, 2022

The reply for the author of #1068

I know, that it is impossible to write one byte to AVR, because ISP commands are two bytes oriented. But if You write program where You use const string located in PROGMEM(FLASH), there is possible that hex or bin contains odd number of bytes. And Avrdude behave not clear, people thinks that MCU is corrupted, and try to switch it to "good" one.
As I see a solution:

Throw an exception like "don't support odd number of bytes in bin/hex file".
Write 0xFF as second byte, if only first is defined.

@mcuee
Copy link
Collaborator

mcuee commented Aug 24, 2022

@dl8dtl
What is your stand on the suggestion?

  1. Throw an exception like "don't support odd number of bytes in bin/hex file".
    or
  2. Write 0xFF as second byte (pad the hex/bin file).

@MCUdude
Copy link
Collaborator

MCUdude commented Aug 25, 2022

What is your stand on the suggestion?
Throw an exception like "don't support odd number of bytes in bin/hex file".
or
Write 0xFF as second byte (pad the hex/bin file).

I'll assume padding the second byte with 0xff is not problematic and would be the most elegant solution. Having the user to modify their hex file by hand is not very user friendly IMO.

@stefanrueger
Copy link
Collaborator

@mariusgreuel There is a current PR out that does a superset of this PR (I think!). Could you have a look at PR #1115 and see whether that one is OK to merge. If so, I'd suggest to close this PR #825.

@mcuee
Copy link
Collaborator

mcuee commented Oct 17, 2022

PR #1115 has been merged.

@stefanrueger
Copy link
Collaborator

Closed as superseded by PR #1115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants