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

Small typo in 'afterburner.c' #63

Open
hubertushirsch opened this issue Mar 24, 2024 · 4 comments
Open

Small typo in 'afterburner.c' #63

hubertushirsch opened this issue Mar 24, 2024 · 4 comments

Comments

@hubertushirsch
Copy link
Contributor

hubertushirsch commented Mar 24, 2024

In line 188 in the printHelp() function the serial baud rate used is incorrectly specified.
The help text says 38400 but the code uses 57600.

In addition in printHelp(), the default device (if no -d 'device' option is specified) is not always COM1 (on Windows), but the highest COM number found (since the extension in serial_port.h).

Hubert

@ole00
Copy link
Owner

ole00 commented Apr 3, 2024

Thanks for the report. It should be now fixed. See commit 0761907

@hubertushirsch
Copy link
Contributor Author

Hi, thanks for the fix.

I have now tried the following change. It works on Windows. I don't have a Linux system to test it with.

static void printHelp() {
    if (!deviceName) {
        serialDeviceGuessName(&deviceName);
    }
    printf("Afterburner " VERSION_EXTENDED "  a GAL programming tool for Arduino based programmer\n");
    .....
    printf("  -d <serial_device> : name of the serial device. Without this option the device %s is tried.\n", deviceName ? deviceName : DEFAULT_SERIAL_DEVICE_NAME);
    printf("                       serial params are: 57600, 8N1\n");
    .....

In your code I keep noticing the inconsistent use of the '0' value. This contradicts the rules of C/C++ syntax and the program is also more difficult to understand for co-programmers. Even if it technically works.

0 is the value zero for a numeric variable.
If you don't want a pointer to point to a valid memory location (variable), you assign it the value NULL or test for NULL if you want to know whether the pointer points to a valid memory location.
For char variables or char arrays, '\0' should be used for the null character.

e.g. in afterburner.c

char verbose = 0;
char* filename = 0;
char* deviceName = 0;
char* pesString = NULL;

should be

char verbose = 0;
char* filename = NULL;
char* deviceName = NULL;
char* pesString = NULL;

or in serial_port.h

  //strip the new-line trailing markers
  if (guessedSerialDevice[textLen - 1] == '\n' || guessedSerialDevice[textLen - 1] == '\r') {
      guessedSerialDevice[textLen - 1] = 0;
  }

should be

 //strip the new-line trailing markers
  if (guessedSerialDevice[textLen - 1] == '\n' || guessedSerialDevice[textLen - 1] == '\r') {
      guessedSerialDevice[textLen - 1] = '\0';
  }

because guessedSerialDevice is a char array that should then be initialized like this:

char guessedSerialDevice[512] = {'\0'};

Greetings

@hubertushirsch
Copy link
Contributor Author

I forgot to comment the line

char verbose = 0;

verbose is used in the program as a pure YES/NO flag. So verbose doesn't actually contain a character as a value. For better understanding, it is a Boolean variable that only has the two values true and false.
You could also write it like this for better readability:

#include <stdbool.h>
bool verbose = false;
.....
  } else if (strcmp("-v", param) == 0) {
    verbose = true;
.....

This also applies to functions that you declared as char, which only return 0 or 1 as a result for successful or failed.

bool function(args) {       // instead of char function(args)
   ....
   return true; // or false // instead of return 1; // or 0
}

Greetings
Hubert

@Emile666
Copy link

Very good comments! While adding them to my local files, I found the following bug:
L.638: "if (verbose & bigRam) {", this should be: "if (verbose && bigRam) {". A single & is a bitwise operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants