Generic USB serial device #1818
Conversation
|
wow! i'll be able to review this code soon, thank you for investigating! |
|
Awesome. I’ll try to adapt my serial PTZ script to this API sometime this week. |
| } else { | ||
| fprintf(stderr, "dev_monitor: unmatched TTY device %s at %s\n", name, node); | ||
| const char *vendor, *model; | ||
| vendor = udev_device_get_property_value(dev, "ID_VENDOR"); | ||
| model = udev_device_get_property_value(dev, "ID_MODEL"); |
There was a problem hiding this comment.
Reiterating serial-number comment from previously, and remembering some serial devices have multiple channels. Can't remember if this has to be selected via a channel or index with udev.
There was a problem hiding this comment.
Hmm, it would be helpful to have an example of this. I added the interface number from ID_USB_INTERFACE_NUM to the serial config args. Based on some googling it seems like devices with multiple serial ports might use that to distinguish between ports? Let me know if that seems right to you!
There was a problem hiding this comment.
Note sure! I have a 4-channel FTDI module that I can test with to verify.
|
@tlubke Thanks for the feedback! I changed up how the device is configured from Lua based on some best practices that I found. |
matron/src/device/device_serial.c
Outdated
|
|
||
| unsigned int us = floor(SECOND * max_read / chars_per_second); | ||
| // latency becomes noticeable above 1ms | ||
| if (us > MILLISECOND) { |
There was a problem hiding this comment.
This (and the thread loop) actually might need a bit of review. I think this probably should return rate of microseconds per byte.
i.e.
...
double bits_per_symbol = char_size + stop_bits + parity_bits;
double bits_per_second = baud_rate * bits_per_symbol;
double bytes_per_second = bits_per_second / 8.0;
return ceil(1e6 / bytes_per_second);
}
Then, when usleep() is called, you can either pass in that value or (max_read * microseconds_per_byte). I'm not sure which is correct because the former may take up too much execution time, but would catch a one byte transfer immediately.
Haven't had a chance to test yet, but I think there may also be an implicit block happening in the read() call. Maybe you could add a print statement and see what the clock reading is before and after that call. Maybe there is no need for usleep() in the first place if the thread can suspend while waiting for a complete line from the serial device.
Another option is to just hardcode the sleep to 46 μs which I think should cover the maximum byte rate.
There was a problem hiding this comment.
Good point, there is a block in read(). I put some timers in there and it looks like it blocks for half a second if there's no data, and otherwise reads as fast as data comes in (between 40us - 40ms testing at 115200 baud, CS8 + 1 stop bit). After testing thread cpu time with and without the usleep, it seems like it does slightly better without the sleep, so I opted to remove the sleep!
| function _norns.serial.match(vendor, model, serial_num, interface_num) | ||
| for id, handler in pairs(serial._handlers) do | ||
| if handler.match(vendor, model, serial_num, interface_num) then |
There was a problem hiding this comment.
I'm thinking about passing these attributes to handler.match in a readonly table instead of as positional arguments. That might make this a little easier to manage if attributes need to be added or removed in the future.
| function _norns.serial.configure(handler_id, tio) | ||
| return serial._handlers[handler_id].configure(tio) |
There was a problem hiding this comment.
When I was writing handler.match I was thinking it would be useful for it to be able to handle a family of devices that might have different attributes but generally speak the same language. However, handler.configure currently doesn't have the same flexibility because all it receives is the termios config. I've been wondering if I need to pass in the same attributes that handler.match receives, or give handler.match the ability to return some sort of user data that gets passed through to handler.configure.
Maybe it's not a big deal though. I think users could work around this limitation by defining multiple serial handlers, but it might be a bit of a headache to wrap that up in a nice interface for scripts.
| @@ -36,6 +36,7 @@ local hooks = { | |||
| script_pre_init = Hook.new(), | |||
| script_post_init = Hook.new(), | |||
| script_post_cleanup = Hook.new(), | |||
| system_pre_device_scan = Hook.new(), | |||
There was a problem hiding this comment.
In hindsight maybe this hook isn’t necessary? I could just call serial.handler in the global scope of the mod since mods are loaded as the last step of startup script.
Adds a new device type,
DEV_TYPE_SERIAL, which can be configured from Lua, as discussed in #1816. FYI I am far from being a C/Linux/serial expert.An example for the Playdate by Panic can be found here: midouest/playdate-norns
Design
Added a new
system_pre_device_scanhook that gets called after startup but before the device scan and event loop. This is intended to allow mods to configure new device handlers before the devices are scanned. I didn't want to mess around with re-scanning devices after new handlers are added or allowing handlers to be removed, but that would be another possible design.When an unmatched TTY is found, the device path, name, vendor and model are passed to weaver which checks each serial device handler for a matching vendor/model. This would allow a single handler to match multiple device models. If the handler matches the device attributes, another configuration callback gets called to configure the serial connection.