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

[Core] Split ChibiOS usart split driver in protocol and hardware driver part #16669

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Mar 16, 2022

This PR builds upon #15907 and #16647 for review please ignore the commits belonging to these.

Description

Goal of this PR is to re-use the protocol logic of the existing ChibiOS usart driver for other means of transportation. Drivers implementing the protocol just need to provide the following methods to work:

/**
 * @brief Clears any intermediate sending or receiving state of the driver to a known good
 * state. This happens after errors in the middle of transactions, to start with
 * a clean slate.
 */
void serial_transport_driver_clear(void);

/**
 * @brief Driver specific initialization on the slave half.
 */
void serial_transport_driver_slave_init(void);

/**
 * @brief Driver specific specific initialization on the master half.
 */
void serial_transport_driver_master_init(void);

/**
 * @brief  Blocking receive of size * bytes.
 *
 * @return true Receive success.
 * @return false Receive failed, e.g. by bit errors.
 */
bool __attribute__((nonnull, hot)) serial_transport_receive(uint8_t* destination, const size_t size);

/**
 * @brief Blocking receive of size * bytes with an implicitly defined timeout.
 *
 * @return true Receive success.
 * @return false Receive failed, e.g. by timeout or bit errors.
 */
bool __attribute__((nonnull, hot)) serial_transport_receive_blocking(uint8_t* destination, const size_t size);

/**
 * @brief Blocking send of buffer with timeout.
 *
 * @return true Send success.
 * @return false Send failed, e.g. by timeout or bit errors.
 */
bool __attribute__((nonnull, hot)) serial_transport_send(const uint8_t* source, const size_t size);

The first successful use-case is the usage of the PIO peripheral on the RP2040 MCU to implement Half-duplex and Full-duplex UART split keyboard communication.

I have tested these changes successfully with both SIO, SERIAL and PIO drivers.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna
Copy link
Member

drashna commented Mar 16, 2022

This depends on the other prs mentioned, correct?

@KarlK90
Copy link
Member Author

KarlK90 commented Mar 16, 2022

Correct :-)

@drashna drashna added the awaiting_pr Relies on another PR to be merged first label Mar 16, 2022
@drashna drashna requested a review from a team March 16, 2022 20:24
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch from 55dcd56 to 6b2e599 Compare March 18, 2022 23:48
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch from 6b2e599 to 469df31 Compare May 16, 2022 20:43
Copy link
Contributor

@preisi preisi left a comment

Choose a reason for hiding this comment

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

I've used this PR for a "quick-and-dirty" implementation of an FDCAN-driver.
The given serial_protocol-driver interface is simple and creating my own implementation worked just fine.
(see preisi@c91e6b5)

platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.h Outdated Show resolved Hide resolved
platforms/chibios/drivers/serial_protocol.c Outdated Show resolved Hide resolved
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch 4 times, most recently from f8011bc to 315fbd8 Compare June 14, 2022 08:16
@KarlK90
Copy link
Member Author

KarlK90 commented Jun 14, 2022

@preisi Thanks for the feedback and recommendations, I integrated those 👍

@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch 2 times, most recently from a6332ee to a070158 Compare June 16, 2022 11:35
@KarlK90
Copy link
Member Author

KarlK90 commented Jun 16, 2022

Two notable changes that might be missed in review:

  1. The previous handshake magic number 0x7 was replaced with the current number of registered transaction entries NUM_TOTAL_TRANSACTIONS this provides some simple sanity check that both firmware versions are identical. (this will fail if the transactions differ in content but not in number though)
  2. The VLA in send for half-duplex serial was replaced with a fixed size buffer:

from

uint8_t dump[size];
return receive(dump, size);

to

uint8_t dump[64];
size_t  bytes_left = size;

while (unlikely(bytes_left > 64)) {
    if (unlikely(!receive(dump, 64))) {
         return false;
     }
     bytes_left -= 64;
}
receive(dump, bytes_left);

Copy link
Contributor

@preisi preisi left a comment

Choose a reason for hiding this comment

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

Looks good to me

size_t bytes_left = size;

while (unlikely(bytes_left > 64)) {
if (unlikely(!receive(dump, 64))) {
Copy link
Member

Choose a reason for hiding this comment

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

Will need to #include "util.h" if it's not already present.

Suggested change
if (unlikely(!receive(dump, 64))) {
if (unlikely(!receive(dump, MIN(bytes_left, 64)))) {

Prevents reading extra if the slave just happens to respond within SERIAL_USART_TIMEOUT.

Copy link
Member Author

@KarlK90 KarlK90 Jun 16, 2022

Choose a reason for hiding this comment

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

I don't think that change is correct? This branch will only be executed if bytes_left > 64 so we can safely always read 64 byte. But I could make the condition bytes_left >= 64.

Comment on lines 33 to 49
bool __attribute__((nonnull, hot)) receive(uint8_t* destination, const size_t size);

/**
* @brief Blocking receive of size * bytes with an implicitly defined timeout.
*
* @return true Receive success.
* @return false Receive failed, e.g. by timeout or bit errors.
*/
bool __attribute__((nonnull, hot)) receive_blocking(uint8_t* destination, const size_t size);

/**
* @brief Blocking send of buffer with timeout.
*
* @return true Send success.
* @return false Send failed, e.g. by timeout or bit errors.
*/
bool __attribute__((nonnull, hot)) send(const uint8_t* source, const size_t size);
Copy link
Member

Choose a reason for hiding this comment

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

Kiiiiiinda allergic to generic names like these.
What about:

  • receive => serial_transport_receive
  • receive_blocking => serial_transport_receive_blocking
  • send => serial_transport_send

......just don't get me started about the naming of serial.

Copy link
Member Author

@KarlK90 KarlK90 Jun 16, 2022

Choose a reason for hiding this comment

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

Ah yes, forgot about C-style namespacing. Will apply that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, applied the prefix to all public methods that are part of the protocol.

@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch from a070158 to d3e5ff3 Compare June 16, 2022 22:44
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch from d3e5ff3 to f3b24f7 Compare June 17, 2022 10:17
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch 2 times, most recently from 0fa88da to fd34cdd Compare June 17, 2022 10:38
@github-actions github-actions bot removed the keymap label Jun 17, 2022
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch 2 times, most recently from 51832ee to 293ecbf Compare June 17, 2022 19:59
This allows other means of split communication to be implemented by
simply implementing the driver specific methods used in the serial protocol.
@KarlK90 KarlK90 force-pushed the feature/serial-protocol branch from 293ecbf to 7d5cb5e Compare June 17, 2022 20:07
@KarlK90 KarlK90 requested a review from tzarc June 17, 2022 20:07
@tzarc tzarc merged commit fe680a8 into qmk:develop Jun 17, 2022
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_pr Relies on another PR to be merged first core documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants