M485 command for RS485 support in Marlin

[Jnesselr]

Description

RS485 is a bus specification, similar to a CAN bus. It uses a differential pair to transmit data. We're using Marlin for the Lumen PnP and controlling it through OpenPnP. Marlin mostly acts as a passthrough here, sending the data to the RS485 bus and listening for responses or timing out.

This PR:

• Uses the RS485_ENABLED line define to enable or disable this feature.
• Adds the M485 command to send data to an RS485 bus.
• Accepts and verifies any responses that come back from the bus and relays them to the host.
• Uses a library to keep Marlin cleaner functionality wise.

Most of this PR is around making sure the serial port is set up correctly. Most of the logic involved is in the M485 command itself or in our library.

Requirements

This requires a board that has an RS485 transceiver such as the Opulo's motherboard for the Lumen PNP. This is to support feeders more than anything, right now. You can see more about the feeders here: https://docs.opulo.io/feeders/1-overview/feeder-overview/

Benefits

This adds the M485 command to send RS485 packets and receive responses. Currently it's hardcoded to be the Photon firmware's packet format, but the idea is for that to be changeable, we just weren't sure how best to implement it and decided against doing so until there are other examples.

One nice aspect to this is that the only thing Marlin cares about is whether the packet that goes back to the host is well formatted, matches checksums, etc. It itself does not generate any packet data. See the OpenPnP PR below for more info.

Configurations

Configurations pending PR to configurations repo.

Related Issues

OpenPnP PR: openpnp/openpnp#1539
Photon Firmware (runs on the feeder): https://github.com/photonfirmware/photon

[thinkyhead]

Everything looks good to me! I moved the new config options to the same section as the other serial ports, removed the need to define RS485_ENABLED (just looks for the serial port define), and did some basic fixes, cleanup, and optimization. I have only one burning question, posted above. I look forward to checking out your rs485 code and –if it would be useful– I can give that a quick review also.

[thinkyhead]

Another 'burning question'…. Is a board required to have RS485-specific hardware, with specific pins assigned to it? If that is the case, then we can sanity-check the RS485_SERIAL port number and make sure it corresponds to the required hardware port.

[GMagician]

Just my 2c. This is the first time I see a RX enable pin on 485 serial communications, isn't it a resources waste?
RS485 is half duplex then you are transmitting you may not receive, then a master to slave protocol should be implemented, then to correclty handle communications answers, just purge RX buffer after TX.
I'm wrong or am I missing something ?

[Jnesselr]

Is a board required to have RS485-specific hardware, with specific pins assigned to it?

Yep!

If that is the case, then we can sanity-check the RS485_SERIAL port number and make sure it corresponds to the required hardware port.

I'm not sure how to do that and my need your help.

@GMagician "resource waste" is always debatable. The code this PR uses already has a controller/peripheral protocol. Our long term goal is to make it so not even that is required and we can do cool things like having an autoreporter automatically report up to the host when there's a valid RS485 message, even if that isn't a response to an M485 command. So we're fine with the "waste" as it is now.

[dna-topoisomerase]

Not sure how important this is, but I don't think this works with G-code compression in OpenPnP, which removes all spaces. When the data to send begins with a decimal digit (e.g. M485 0...), the code number is no longer parsed as 485.

[Jnesselr]

I look forward to checking out your rs485 code and –if it would be useful– I can give that a quick review also.

Repo: https://github.com/Jnesselr/RS485/

I don't think this works with G-code compression in OpenPnP, which removes all spaces.

Yep. We did test it extensively, but yes if you have either "remove comments" or "compress gcode" on in OpenPnP, then it will screw this up. Remove comments is unfortunately more aggressive than just removing the comments themselves and does the same code as "compress gcode" here. They're honestly just not worth having it on, given the amount of data going through.

[Jnesselr]

Also, @thinkyhead, I'm not 100% sure why it's failing. The errors I'm seeing:

/home/runner/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: .pio/build/Opulo_Lumen_REV3/src/src/MarlinCore.cpp.o: in function `setup':
/home/runner/work/Marlin/Marlin/Marlin/src/MarlinCore.cpp:1666: undefined reference to `rs485_init()'
/home/runner/.platformio/packages/toolchain-gccarmnoneeabi/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: .pio/build/Opulo_Lumen_REV3/src/src/gcode/gcode.cpp.o: in function `GcodeSuite::process_parsed_command(bool)':
/home/runner/work/Marlin/Marlin/Marlin/src/gcode/gcode.cpp:895: undefined reference to `GcodeSuite::M485()'

I'm not 100% sure why this is failing. It looks like the original failure started with a merged branch and now MarlinCore.cpp is conflicting. I'll look into it more and do some more debugging, but I'm hoping the issue is something obvious to you since I'm not in a state where I can compile and test this code and it visually looks correct to me.

[theacodes]

To make it easier for control programs to parse RS-485 responses, it'd be nice for the various messages this can print to be self-consistent and consistent with the i2c commands. For example, right now this can return a variety of messages:

rs485-reply: TIMEOUT
rs485-reply: 001122334455
rs485-unexpected-packet: 001122334455
Error:String must contain an even number of bytes.
Error:String too long (32 bytes max).
Error:RS485: Write failed interrupted

Compared to i2c:

echo:i2c-reply: from:12 bytes:3 data:01 02 03
echo:Bad I2C address (8-127)

I propose the following:

echo:rs485-timeout
echo:rs485-reply: 001122334455
echo:rs485-unexpected-packet: 001122334455
Error:String must contain an even number of bytes.
Error:String too long (32 bytes max).
echo:rs485-error: interrupted

[thinkyhead]

Note that messages starting with "Error:" may cause hosts like OctoPrint to halt and disconnect. If that is not desired behavior, messages should not start with "Error:". We now have the option to emit a less severe Warning: message.

[thinkyhead]

I get the feeling we're arriving at the station. In ten, nine, eight, …

[Jnesselr]

We're definitely close, but still need to go over a couple of things.

@theacodes (Sorry, completely missed your message) I'd really like to avoid changing the output that is given with the rs485-reply, but mostly because of the extra work on the OpenPnP side. We already have OpenPnP regex reading off of that, and changing it here would require us to do a lot more work there to handle different types of replies. See here for the regex. We don't currently handle some of the other error conditions directly, we use OpenPnP retries to manage them.

If you can help figure out how to make the rs485 regex work in OpenPnP and @sphawes is okay with the support burden of that (meaning helping customers ensure what Marlin sends and what OpenPnP receives is the same), I'm game for it. Best guess would be rs485-(?<Value>.*) which might be too aggressive, but no worse than what is already in there.

The warning vs error thing is a little interesting to me. We don't go through OctoPrint for this, but I can understand why this functionality may eventually be something that's used through OctoPrint. Right now, other commands I looked at do send back errors but only if their input is bad. So some of our responses should still be errors, but the rs485_write_failed shouldn't be an error. If we change that one to like the echo:rs485-error: interrupted that @theacodes suggested, is that okay?

[theacodes]

I am hesitant to tie implementation details here to OpenPnP, since it won't be the only host program using this functionality. @sphawes and I can write documentation for folks when they update their Lumen mobo FW.

My biggest concern with the output formats is just being consistent and being easy to match without having to do nested matching- the main problem being rs485-reply: TIMEOUT and rs485-reply: {data}. This means I can't solely match on rs485-reply.

I know I said I didn't want to tie this to the implementation details of any one consumer, but just as an illustration- the code from our internal internal tool, Glimmer, looks like this:

def _decode_response(line: str) -> bytes:
    prefix, rest = line.split(":", 1)
    rest = rest.strip()

    match prefix:
        case "rs485-reply":
            if rest == "TIMEOUT":
                raise RS485TimeoutError()

            try:
                return bytes.fromhex(rest)
            except ValueError as err:
                raise RS485ReadError(...) from err

        case "rs485-unexpected-packet":
            raise RS485UnexpectedPacketError(...)

        case "Error":
            raise RS485WriteError(rest)

        case _:
            raise RS485Error(...)

Where my preference would be this:

def _decode_response(line: str) -> bytes:
    prefix, rest = line.split(":", 1)
    rest = rest.strip()

    match prefix:
        case "rs485-reply":
            try:
                return bytes.fromhex(rest)
            except ValueError as err:
                raise RS485ReadError(...) from err
            
        case "rs485-timeout":
            raise RS485TimeoutError()

        case "rs485-unexpected-packet":
            raise RS485UnexpectedPacketError(...)

        case "rs485-error":
            raise RS485WriteError(...)

        case _:
            raise RS485Error(...)

[sphawes]

I'm ok with needing to tie a Marlin version to a certain OpenPnP vacuum actuator read regex. It's a pretty simple copy-paste to update, and as long as we change both in lock-step, most folks will never encounter the difference. I definitely prefer to try to standardize our response format in an easy to parse way instead of preserving the current OpenPnP configuration.

[thinkyhead]

If you do want to preserve a regex for the old pattern and also recognize the new pattern, I'm happy to help. I'm one of those mad nerds who enjoys making things like fancy regexes, proper CSS, and elaborate pseudo-justifications.

[thinkyhead]

I believe we've sorted out the remaining questions about the protocol changes in this PR. If there are no other concerns I'll go ahead and merge this today.

[Jnesselr]

Please hold off just a little longer. Several of us are at Open Sauce and Stephen needs to make a couple of small changes more.

[sphawes]

thank you for waiting to merge @thinkyhead!

I talked through the implications of changing the format with @Jnesselr and @theacodes, and I think that it would be pretty frustrating for users if we change the response format. it's not just a simple regex change in OpenPnP as i thought in my previous comment; it would actually require a new build of OpenPnP to handle the new format correctly.

I propose we go back to the format in f1afaa1. @thinkyhead if this is acceptable to you, i can revert the format but keep your other changes and we can merge.

[thinkyhead]

I propose we go back to the format

Will there be future updates to OpenPnP to allow for a different protocol format and additional features? If so, we should add a version specifier. The option #define M485_PROTOCOL 1 would build the old protocol, and version 2 would build the new (proposed) protocol that applies to some future version of OpenPnP. And as the OpenPnP protocol gets more changes and new features we can bump the version of the protocol to take advantage of those new features.

How about that idea?

[sphawes]

That sounds great to us. Thank you @thinkyhead! I'll take my best crack at implementing this for your review if that works.

[thinkyhead]

I'll take my best crack at implementing this for your review if that works.

You could do that! (or ask ChatGPT to do it….) I can go ahead and whip together the skeleton of the idea now, and then you should go ahead and adjust it to your preferences.

[Jnesselr]

I haven't run that commit's code, but quick check and it looks correct. Not sure what defaults should be.

[sphawes]

@thinkyhead thank you, I've just tested this in OpenPnP with protocol 1 and it works as expected. I'm all good for this to merge whenever! Thank you so much for your tremendous help getting this in, Scott.