Add Mode 5: Physical reset, for UARTS with only RTS, no DTR. by SmittyHalibut · Pull Request #28 · msalau/rl78flash

SmittyHalibut · 2024-04-04T17:53:56Z

This is a draft pull request so I can comment on the sum of two commits.

confirmed operation works with reading CTS (mode 5)

SmittyHalibut · 2024-04-04T17:54:38Z

    "\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"


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

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.)

SmittyHalibut · 2024-04-04T17:55:28Z

+                // 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);


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

SmittyHalibut · 2024-04-04T17:56:19Z

+        if (mode == 4)
+        {
+            mode = 4;


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

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.

SmittyHalibut · 2024-04-04T18:02:39Z

 {
+    if (mode == 4) // operating in mode 5
+    {
+        printf("No GPIO SW reset available, must use switch\r\n");


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.

SmittyHalibut · 2024-04-04T18:04:14Z

+    {
+        printf("No GPIO SW reset available, must use switch\r\n");
+    }
+    else {


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

But fix the indentation below.

SmittyHalibut · 2024-04-04T18:04:27Z

    {
        serial_set_dtr(fd, level);
    }
+    }


Indentation is broken down to here.

SmittyHalibut · 2024-04-04T18:04:45Z

    unsigned char r;
+    //(mode = desired communication mode from terminal input - 1)
+
+   // Determine UART mode from mode argument


Indentation (just the one line)

SmittyHalibut · 2024-04-04T18:05:01Z

        r = SET_MODE_1WIRE_UART;
        communication_mode = 1;
    }
+


Remove blank line.

SmittyHalibut · 2024-04-04T18:07:36Z

    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");
+        }
+        else
+        {
        printf("Using communication mode %u%s\n",
               (mode & (MODE_UART | MODE_RESET)) + 1,
               (mode & MODE_INVERT_RESET) ? " with RESET inversion" : "");
+        }   
    }


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. :-)

SmittyHalibut · 2024-04-04T18:12:44Z

+        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;
+            }
+        }


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.

SmittyHalibut · 2024-04-04T18:25:03Z

    }
    serial_write(fd, &r, 1);
-    if (1 == communication_mode)
+    if ((1 == communication_mode) | (5 == communication_mode))


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.

SmittyHalibut · 2024-04-04T18:26:02Z

 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;


Too much indentation. Fix this.

SmittyHalibut · 2024-04-04T18:35:34Z

+        serial_flush(fd);
+
+    }
+    else if ((mode == 0) | (mode == 1) | (mode == 2))


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

Actually, this is just else.

SmittyHalibut · 2024-04-04T19:00:45Z

            if (1 <= verbose_level)
            {
-                printf("Reset MCU\n");
+                if (mode == 4) // operating in mode 5


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.

SmittyHalibut · 2024-04-04T19:01:01Z

+                    printf("Resetting MCU...\n");
+                }
+            }
+            if (mode != 4) // Case for HW reset switch


MODE_RESET_INPUT

SmittyHalibut · 2024-04-04T19:01:51Z

 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


MODE_RESET_INPUT

SmittyHalibut · 2024-04-04T19:05:08Z

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


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

SmittyHalibut · 2024-04-04T19:16:57Z

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

    if (MODE_RESET_RTS == (mode & MODE_RESET))


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)))

SmittyHalibut · 2024-04-04T19:19:34Z

+        if (mode == 4) // Special case for mode 5
+        {
+            communication_mode = 5;
+            printf("Using communication mode 5 (HW RESET with CTS reading RESET)\n");
+        }


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.

SmittyHalibut · 2024-04-04T19:21:44Z

-    serial_set_txd(fd, 0);                                  /* TOOL0 -> 0 */
-    if (wait)
+    // Begin reset procedure
+    if (mode == 4) // sequencing for mode 5


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.)

SmittyHalibut · 2024-04-04T19:30:37Z

    int ret = serial_write(fd, buf, sizeof buf);
    // Read back echo
-    if (1 == communication_mode)
+    if ((1 == communication_mode) | (5 == communication_mode))


Same as above. Only test for 1 ==

SmittyHalibut · 2024-04-04T19:35:17Z

+// 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


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) ...

SmittyHalibut

whew! Ok.

There are a few style and formatting comments buried in here, don't lose those.

But the biggest change I'm requesting is that we continue to treat mode as a bit-map, instead of just a sequentially assigned arbitrary mode number. Modes 1 through 4 are actually a bit map of features, one of which matters to us. So I propose we keep with that bit map, and add another bit that is "Is Reset an input or not?" When we continue to use bit 1 to determine whether it's a 1-wire or 2-wire protocol, and bit 2 to determine which output pin to use for reset (which doesn't matter in our case).

OR! Possibly use that bit to select between CTR and DSR for the input pins. shrug or don't bother with that, and just stick to CTR.

In any case, most of my actual code changes in this review are to implement this change.

- Replaced references to mode numerically with references to bitmap - Style fixes

Jeak added 2 commits March 31, 2024 21:20

Add of hardware reset and CTS monitoring reset functionality

13795e5

Bug fixes

3a1cba0

confirmed operation works with reading CTS (mode 5)

SmittyHalibut commented Apr 4, 2024

View reviewed changes

Comment thread src/rl78.c Outdated

{

serial_set_dtr(fd, level);

}

}

Copy link
Copy Markdown

Author

SmittyHalibut Apr 4, 2024

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.

SmittyHalibut commented Apr 4, 2024

View reviewed changes

Code cleanup to address comments in PR

896538f

- Replaced references to mode numerically with references to bitmap - Style fixes

Conversation

SmittyHalibut commented Apr 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmittyHalibut left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading

SmittyHalibut Apr 4, 2024 •

edited

Loading