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.

Sign up for GitHub

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

Add Mode 5: Physical reset, for UARTS with only RTS, no DTR. #28

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add Mode 5: Physical reset, for UARTS with only RTS, no DTR. #28

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13795e5
Add of hardware reset and CTS monitoring reset functionality
Jeak Apr 1, 2024
3a1cba0
Bug fixes
Jeak Apr 3, 2024
896538f
Code cleanup to address comments in PR
Jeak Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ Features:
* Only S-record image files are accepted as input files;
* RL78/G10 parts can be programmed only in 1-wire mode,
other RL78 parts support both modes (1-wire and 2-wire);
* Reset signal is controlled by DTR or RTS signal.
* Supports software control of reset signal by DTR or RTS signal.
* Supports hardware monitoring of reset signal by observing CTS signal.

# Thanks to

Expand Down
32 changes: 29 additions & 3 deletions src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const char *usage =
"\t\t\tn=2 Two-wire UART, Reset by DTR\n"
"\t\t\tn=3 Single-wire UART, Reset by RTS\n"
"\t\t\tn=4 Two-wire UART, Reset by RTS\n"
"\t\t\tn=5 Single-wire UART, Hardware Reset, reset state read by CTS (modified mode 1/3)\n"
Copy link
Author

Choose a reason for hiding this comment

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

It's all "hardware reset." I'd say "Manual reset button" instead.

Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will probably want to update this after seeing below about mode being a bitmap. n=5 is as you describe, but n=6 will be Two-wire UART, Manual Reset. (You get it for free when mode is a bitmap.)

"\t\t\tdefault: n=1\n"
"\t-P n\tSet protocol version\n"
"\t\t\tn=-1 Try to autodetect the protocol version from the unit's Silicon Signature\n"
Expand Down Expand Up @@ -117,14 +118,21 @@ int main(int argc, char *argv[])
break;
case 'm':
mode = strtol(optarg, &endp, 10) - 1;
// mode = desired communication mode from terminal input - 1
if (optarg == endp
|| MODE_MAX_VALUE < mode
|| MODE_MIN_VALUE > mode)
{
fprintf(stderr, "Invalid mode\n");
printf("%s", usage);
// For debug
//printf("Communication mode passed to rl78 funcs: %i\r\n", mode);
//printf("Mode value expression: %i\r\n",mode_val_expr);
return EINVAL;
}
// For debug
//printf("Communication mode passed to rl78 funcs: %i\r\n", mode);
//printf("Mode value expression: %i\r\n",mode_val_expr);
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Normally, you'd remove your temporary debugging code from the commit before issuing a PR. Or, wrap it in if (verbose_level > whatever) block.

break;
case 't':
terminal = 1;
Expand Down Expand Up @@ -222,7 +230,16 @@ int main(int argc, char *argv[])

if (invert_reset)
{
mode |= MODE_INVERT_RESET;
if (mode == 4)
{
mode = 4;
Copy link
Author

Choose a reason for hiding this comment

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

This code is only ever run if mode == 4 so assigning it to 4 is unnecessary.

Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, treating mode as a bitmap (see below), you might as well still allow this, and use it when detecting CTS.

That is, just remove this change and go back to the original code. It's ok if MODE_INVERT_RESET bit is set.

printf("Using mode 5. INVERT RESET command will not be applied.\r\n");
}
else
{
mode |= MODE_INVERT_RESET;
}

}
char *portname = NULL;
char *filename = NULL;
Expand Down Expand Up @@ -473,9 +490,18 @@ int main(int argc, char *argv[])
{
if (1 <= verbose_level)
{
printf("Reset MCU\n");
if (mode == 4) // operating in mode 5
Copy link
Author

Choose a reason for hiding this comment

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

When used to determine whether reset is controlled by the code, or triggered by the user, change if (mode == 4) to if (MODE_RESET_INPUT_TRUE == (mode & MODE_RESET_INPUT))

This happens in multiple places. I'll just say MODE_RESET_INPUT in future comments to mean the same thing.

{
printf("Done! Use HW reset switch to reset MCU.\r\n");
}
else {
printf("Resetting MCU...\n");
}
}
if (mode != 4) // Case for HW reset switch
Copy link
Author

Choose a reason for hiding this comment

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

MODE_RESET_INPUT

{
rl78_reset(fd, mode);
}
rl78_reset(fd, mode);
}
}
while (0);
Expand Down
104 changes: 85 additions & 19 deletions src/rl78.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include <string.h>
#include <stdio.h>
#include "wait_kbhit.h"
#include <errno.h>
#include <sys/ioctl.h>

#include "serial.h"
#include "rl78.h"
Expand All @@ -29,7 +31,13 @@ extern int verbose_level;
static unsigned char communication_mode;

static void rl78_set_reset(port_handle_t fd, int mode, int value)
// Set reset signal for MCU based on mode (not applicable for mode 5 with hardware switch)
{
if (mode == 4) // operating in mode 5
Copy link
Author

Choose a reason for hiding this comment

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

MODE_RESET_INPUT

{
printf("No GPIO SW reset available, must use switch\r\n");
Copy link
Author

Choose a reason for hiding this comment

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

Pedantic Pet Peeve of mine: feel free to ignore. ;-)

Switches are latching and/or sliding/rotating, buttons are momentary and/or linear motion. This is a button, not a switch.

Yes, yes, this depends on how you implement the hardware. But I've never seen a Reset Switch, only ever Reset Buttons. Possibly a Reset Momentary Switch on front-panels of old mini-computers. But these are almost certainly going to be buttons.

}
else {
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

GitHub won't let me select the whole block here.

But fix the indentation below.

int level = (mode & MODE_INVERT_RESET) ? !value : value;

if (MODE_RESET_RTS == (mode & MODE_RESET))
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't be messing with RTS or DTR if we are inputing the reset signal.

if ((MODE_RESET_RTS == (mode & MODE_RESET)) && (MODE_RESET_INPUT_FALSE == (mode && MOST_RESET_INPUT)))

Expand All @@ -40,48 +48,105 @@ static void rl78_set_reset(port_handle_t fd, int mode, int value)
{
serial_set_dtr(fd, level);
}
}
Copy link
Author

Choose a reason for hiding this comment

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

Indentation is broken down to here.

}

int rl78_reset_init(port_handle_t fd, int wait, int baud, int mode, float voltage)
// Routine for resetting RL78 and initializing it in programming mode
{
unsigned char r;
//(mode = desired communication mode from terminal input - 1)

// Determine UART mode from mode argument
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indentation (just the one line)

if (MODE_UART_1 == (mode & MODE_UART))
{
r = SET_MODE_1WIRE_UART;
communication_mode = 1;
}

Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Remove blank line.

else
{
r = SET_MODE_2WIRE_UART;
communication_mode = 2;
}
if (4 <= verbose_level)
{
if (mode == 4) // Special case for mode 5
{
communication_mode = 5;
printf("Using communication mode 5 (HW RESET with CTS reading RESET)\n");
}
Copy link
Author

Choose a reason for hiding this comment

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

Don't change communication_mode here. I think we want this to stay either 1 or 2. You can still output the verbose message, but trigger that based on (mode & MODE_RESET_INPUT == MODE_RESET_INPUT_TRUE instead of mode == 4.

else
{
printf("Using communication mode %u%s\n",
(mode & (MODE_UART | MODE_RESET)) + 1,
Copy link
Author

Choose a reason for hiding this comment

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

(mode & (MODE_UART | MODE_RESET | MODE_RESET_INPUT)) + 1

(mode & MODE_INVERT_RESET) ? " with RESET inversion" : "");
}
}
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is why the behavior changes when you change your verbosity level. communication_mode = 5 shouldn't be inside if (4 <= verbose_level)

Or rather, you shouldn't be setting communication_mode at all, which is why it broke when you increased the verbosity. :-)

rl78_set_reset(fd, mode, 0); /* RESET -> 0 */
serial_set_txd(fd, 0); /* TOOL0 -> 0 */
if (wait)
// Begin reset procedure
if (mode == 4) // sequencing for mode 5
Copy link
Author

Choose a reason for hiding this comment

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

if (MODE_RESET_INPUT_TRUE == (mode & MODE_RESET_INPUT))

(ngl, I don't understand putting the constant before the == thing. But maintaining code style is more important than using MY code style.)

{
printf("Turn MCU's power on and press any key...");
wait_kbhit();
printf("\n");
//printf("Turn MCU's power on with RESET low. Press any key to continue...\n");
//wait_kbhit();
serial_set_txd(fd, 0);
printf("Press and hold RESET.\n");
int reset = serial_get_cts(fd);
if (reset == 1)
{
while (1) // while CTS/RESET is high, wait
{
reset = serial_get_cts(fd);
if (reset == 0)
{
printf("Got reset press. Hang on...\n");
usleep(100000);
printf("Let go of RESET.\n");
break;
}
}
}

while (1) // while CTS/RESET is low, wait
{
reset = serial_get_cts(fd);
if (reset == 1)
{
printf("Got reset release. Attempting to set communication mode...\n");
break;
}
}
Comment on lines +91 to +115
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is overly complex to read.

Also, lets allow for inverted reset here. Add:

bool active = (mode & MODE_INVERT_RESET) ? 0 : 1;

Test my active logic here. I might have it backwards.

Replace the first block with just:

while (serial_get_cts(fd) == active)
{
    usleep(1000);
}

Then move the printf(); usleep(); printf() block outside the while loop.

Replace the second block with:

while (serial_get_cts(fd) != active)
{
    usleep(1000);
}

Then move the printf() outside the while loop.

The usleep(1000) just keeps it from completely locking up a CPU while waiting.

// Reset is now high
serial_flush(fd);
usleep(3000);
serial_set_txd(fd,1); // TOOL0 -> 1
usleep(1000);
serial_flush(fd);

}
else if ((mode == 0) | (mode == 1) | (mode == 2))
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

else if (MODE_RESET_INPUT_FALSE == (mode & MODE_RESET_INPUT))

Actually, this is just else.

{ // sequencing for mode 1,2,3,4
rl78_set_reset(fd, mode, 0); /* RESET -> 0 */
serial_set_txd(fd, 0); /* TOOL0 -> 0 */
if (wait)
{
printf("Turn MCU's power on and press any key...");
wait_kbhit();
printf("\n");
}
serial_flush(fd);
usleep(1000);
rl78_set_reset(fd, mode, 1); /* RESET -> 1 */
usleep(3000);
serial_set_txd(fd, 1); /* TOOL0 -> 1 */
usleep(1000);
serial_flush(fd);
}
serial_flush(fd);
usleep(1000);
rl78_set_reset(fd, mode, 1); /* RESET -> 1 */
usleep(3000);
serial_set_txd(fd, 1); /* TOOL0 -> 1 */
usleep(1000);
serial_flush(fd);
if (3 <= verbose_level)
{
printf("Send 1-byte data for setting mode\n");
}
serial_write(fd, &r, 1);
if (1 == communication_mode)
if ((1 == communication_mode) | (5 == communication_mode))
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

communication_mode is just whether it's a 1-wire or 2-wire. Since mode 5 is a 1-wire, just test for 1 here. This is part of the treating mode as a bit-map change.

Read: Undo this change. Only test for (1 == communication_mode) as originally done.

{
serial_read(fd, &r, 1);
}
Expand All @@ -91,11 +156,12 @@ int rl78_reset_init(port_handle_t fd, int wait, int baud, int mode, float voltag

int rl78_reset(port_handle_t fd, int mode)
{
serial_set_txd(fd, 1); /* TOOL0 -> 1 */
rl78_set_reset(fd, mode, 0); /* RESET -> 0 */
usleep(10000);
rl78_set_reset(fd, mode, 1); /* RESET -> 1 */
serial_set_txd(fd, 1); /* TOOL0 -> 1 */
rl78_set_reset(fd, mode, 0); /* RESET -> 0 */
usleep(10000);
rl78_set_reset(fd, mode, 1); /* RESET -> 1 */
return 0;
Comment on lines 154 to 160
Copy link
Author

Choose a reason for hiding this comment

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

Too much indentation. Fix this.


}

static
Expand Down Expand Up @@ -131,7 +197,7 @@ int rl78_send_cmd(port_handle_t fd, int cmd, const void *data, int len)
buf[len + 4] = ETX;
int ret = serial_write(fd, buf, sizeof buf);
// Read back echo
if (1 == communication_mode)
if ((1 == communication_mode) | (5 == communication_mode))
Copy link
Author

Choose a reason for hiding this comment

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

Same as above. Only test for 1 ==

{
serial_read(fd, buf, sizeof buf);
}
Expand Down
6 changes: 5 additions & 1 deletion src/rl78.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,23 @@
#define RESPONSE_FORMAT_ERROR (-2)
#define RESPONSE_EXPECTED_LENGTH_ERROR (-3)

// serial command data to tell RL78 which serial mode to use for programming
#define SET_MODE_1WIRE_UART 0x3A
#define SET_MODE_2WIRE_UART 0x00

#define RL78_MIN_VOLTAGE 1.8f
#define RL78_MAX_VOLTAGE 5.5f

// Communication modes passed to RL78 funcs
// Mode 1: MODE
#define MODE_UART 1
#define MODE_UART_1 0
#define MODE_UART_2 MODE_UART
#define MODE_RESET 2
#define MODE_RESET_DTR 0
#define MODE_RESET_RTS MODE_RESET
#define MODE_MAX_VALUE (MODE_UART | MODE_RESET)
#define MODE_MAX_VALUE 4
//#define MODE_MAX_VALUE (MODE_UART | MODE_RESET)
#define MODE_MIN_VALUE 0
#define MODE_INVERT_RESET 0x80
Copy link
Author

@SmittyHalibut SmittyHalibut Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok this is the big "mode as a bit-map" thing I've been talking about above.

Bit 1 of mode is whether it's a 1-wire or 2-wire serial protocol.
Bit 2 of mode is whether RESET is on DTR or RTS.

We should make Bit 4 of mode be whether RESET is an INPUT or not. All my comments above are to change the code for this.

Here, we need these #defines to be:

...
#define MODE_RESET_RTS   MODE_RESET
#define MODE_RESET_INPUT   4
#define MODE_RESET_INPUT_FALSE 0
#define MODE_RESET_INPUT_TRUE  MODE_RESET_INPUT
#define MODE_MAX_VALUE  (MODE_UART | MODE_RESET | MODE_RESET_INPUT)
...


Expand Down
21 changes: 21 additions & 0 deletions src/serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <sys/ioctl.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>

extern int verbose_level;

Expand Down Expand Up @@ -145,6 +146,24 @@ int serial_set_dtr(port_handle_t fd, int level)
return ioctl(fd, command, &dtr);
}

int serial_get_cts(port_handle_t fd)
{
int status, flag = 0;
if (ioctl(fd, TIOCMGET, &status) == -1)
{
printf("TIOCMGET failed\n");
}
else {
flag = !(status & TIOCM_CTS);
if (flag == 32) {

flag = 1;

}
}
return(flag);
}

int serial_set_rts(port_handle_t fd, int level)
{
unsigned long command;
Expand All @@ -165,10 +184,12 @@ int serial_set_txd(port_handle_t fd, int level)
unsigned long command;
if (level)
{
// stop sending zero bits (logic high)
command = TIOCCBRK;
}
else
{
// start sending zero bits (logic low)
command = TIOCSBRK;
}
return ioctl(fd, command);
Expand Down
1 change: 1 addition & 0 deletions src/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ port_handle_t serial_open(const char *port);
int serial_set_baud(port_handle_t fd, int baud);
int serial_set_parity(port_handle_t fd, int enable, int odd_parity);
int serial_set_dtr(port_handle_t fd, int level);
int serial_get_cts(port_handle_t fd);
int serial_set_rts(port_handle_t fd, int level);
int serial_set_txd(port_handle_t fd, int level);
int serial_flush(port_handle_t fd);
Expand Down