Polarisru / updiprog

Pure-C utility for programming AVR devices with UPDI interface using a standard TTL serial port
BSD 2-Clause "Simplified" License
37 stars 21 forks source link

Cannot flash due to "Problem reading Hex file" #3

Closed ansemjo closed 4 years ago

ansemjo commented 4 years ago

Description

I'm trying to flash a simple blink example to an ATmega4809, which was compiled with PlatformIO. updiprog cannot read the hex file, however.

After adding a few debug statements, the culprit appears to be the sanity check in IHEX_ReadFile: https://github.com/Polarisru/updiprog/blob/3741ea79390efe499221dc53eb20f5e9bfcb0a55/ihex.c#L148-L149

The left side returns 44 while strlen(str) returns 45.

If I just add another + 1 on the left side as an override, the program is flashed correctly and my LED blinks. Is strlen() handling the CRLF (see below) incorrectly?

I'm on Linux 5.5.4-arch1-1 x86_64.

Sources

src/blink.c:

#define F_CPU 3333333UL

#include <avr/io.h>
#include <util/delay.h>

#define LED 7

int main (void)
{
    PORTA.DIR |= (1 << LED);

    while (1)
    {
        PORTA.OUT ^= (1 << LED);
        _delay_ms(500);
    }
}

.pio/build/mega/firmware.hex:

:100000000C9450000C945A000C945A000C945A0012
:100010000C945A000C945A000C945A000C945A00F8
:100020000C945A000C945A000C945A000C945A00E8
:100030000C945A000C945A000C945A000C945A00D8
:100040000C945A000C945A000C945A000C945A00C8
:100050000C945A000C945A000C945A000C945A00B8
:100060000C945A000C945A000C945A000C945A00A8
:100070000C945A000C945A000C945A000C945A0098
:100080000C945A000C945A000C945A000C945A0088
:100090000C945A000C945A000C945A000C945A0078
:1000A00011241FBECFEFCDBFDFE3DEBF0E945C0097
:1000B0000C946E000C940000809100048068809382
:1000C00000048091040480588093040425E186E1B3
:1000D00095E0215080409040E1F7F3CFF894FFCFB6
:00000001FF

The hexdump shows that lines are terminated with \x0d\x0a, i.e. "CRLF":

$ xxd .pio/build/mega/firmware.hex | head -4
00000000: 3a31 3030 3030 3030 3030 4339 3435 3030  :100000000C94500
00000010: 3030 4339 3435 4130 3030 4339 3435 4130  00C945A000C945A0
00000020: 3030 4339 3435 4130 3031 320d 0a3a 3130  00C945A0012..:10  <<<
00000030: 3030 3130 3030 3043 3934 3541 3030 3043  0010000C945A000C
ansemjo commented 4 years ago

strlen(str) appears to be counting CRLF as two characters, therefore yielding an unexpected length. Completely trimming whitespace first and reducing the expected length by one fixes it for me and should be portable:

diff --git a/ihex.c b/ihex.c
index 860a8ad..642d54f 100644
--- a/ihex.c
+++ b/ihex.c
@@ -1,4 +1,5 @@
 #include <stdio.h>
+#include <ctype.h>
 #include <string.h>
 #include "ihex.h"

@@ -131,6 +132,7 @@ uint8_t IHEX_ReadFile(FILE *fp, uint8_t *data, uint16_t maxlen, uint16_t *max_ad
   uint8_t i;
   uint8_t byte;
   char str[128];
+  char *end;

   addr = 0;
   segment = 0;
@@ -138,6 +140,10 @@ uint8_t IHEX_ReadFile(FILE *fp, uint8_t *data, uint16_t maxlen, uint16_t *max_ad
   {
     if (fgets(str, sizeof(str), fp) == NULL)
       return IHEX_ERROR_FILE;
+    // trim whitespace on the right
+    end = str + strlen(str) - 1;
+    while (end > str && isspace((unsigned char) *end)) end--;
+    end[1] = '\0';
     if (strlen(str) < IHEX_MIN_STRING)
       return IHEX_ERROR_FMT;
     len = IHEX_GetByte(&str[IHEX_OFFS_LEN]);
diff --git a/ihex.h b/ihex.h
index d2f2f83..e96c6cf 100644
--- a/ihex.h
+++ b/ihex.h
@@ -6,7 +6,7 @@
 #include <stdbool.h>

 #define IHEX_LINE_LENGTH    16
-#define IHEX_MIN_STRING     12
+#define IHEX_MIN_STRING     11

 #define IHEX_OFFS_LEN       1
 #define IHEX_OFFS_ADDR      3

(taken from: https://stackoverflow.com/a/122721)

Polarisru commented 4 years ago

Hello, thank you for your message, i will check it with Windows version and commit the fix as soon as possible.

ansemjo commented 4 years ago

I've created a PR #4 for you with the above patch. :)

edit: attaching the source and firmware file if you need it for testing: blink.zip