Hi Paul,
On 24.10.2015 13:19, Paul Kocialkowski wrote:
I will probably send v2 right away, feel free to follow up the discussion on some of these comments in there, it'll probably be easier.
I'll keep some of the general discussion here. Some new comments will come inline with v2.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
Same thing here, spi_send_command *should* fill-in the whole buffer, but in case it doesn't, I'd rather have known values instead, just in case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
This is probably not a hard requirement either, but seems like a good programming habit to produce reliable code to me.
What bothers me is that you imply that spi_send_command() may behave wrong. That is very defensive programming. And if spi_send_command() really does it wrong, we'd never know.
Well, that's generally why I prefer to never initialize unless it's necessary. So many errors out there that we can't see due to over initialization :-/
do {rc = edi_read(flash, ENE_XBI_EFDAT, buf);if (!rc)break;if (!timeout) return ...? or loop for ever!
Thanks, sorry for such carelessness, there are other places where timeout is not checked (even though it's more critical in this spot).
/* Just in case. */while (edi_spi_busy(flash) && timeout--)programmer_delay(10);} while (rc);Redundant check (rc can't be zero when we reach it) hides the endless loop. Nice try! hehe
That's correct, I simply prefer to have a dummy condition loosely related to the code logic (that is never actually reached) because, well, it doesn't hurt and looks better then while (1).
If you believe it obfuscates the code, I'd be fine with removing that form.
Yes, please change that. I believe, a constant condition should always be obvious.
Nico