From 44c5b2bbb1a00cf07e1725c76d1920e8c73977ed Mon Sep 17 00:00:00 2001
From: Jeff Epler <jepler@gmail.com>
Date: Thu, 17 Sep 2020 12:44:40 -0500
Subject: Respond to review comments

Thanks @tannewt!
---
 ports/atmel-samd/common-hal/_canio/Listener.c |  1 -
 shared-bindings/_canio/CAN.c                  | 17 +++--------------
 shared-bindings/_canio/Listener.c             | 10 ++++------
 shared-bindings/_canio/Match.c                |  9 ++++-----
 shared-bindings/_canio/Match.h                |  6 +++++-
 shared-bindings/_canio/Message.c              | 23 +++++++++++------------
 shared-bindings/_canio/Message.h              | 13 +++++++++++++
 shared-module/_canio/Match.c                  |  6 +++---
 shared-module/_canio/Match.h                  |  5 -----
 shared-module/_canio/Message.c                | 20 ++++++++++----------
 shared-module/_canio/Message.h                | 12 ------------
 11 files changed, 53 insertions(+), 69 deletions(-)

diff --git a/ports/atmel-samd/common-hal/_canio/Listener.c b/ports/atmel-samd/common-hal/_canio/Listener.c
index 4d54e9f73..0a5f629f8 100644
--- a/ports/atmel-samd/common-hal/_canio/Listener.c
+++ b/ports/atmel-samd/common-hal/_canio/Listener.c
@@ -375,7 +375,6 @@ bool common_hal_canio_listener_readinto(canio_listener_obj_t *self, canio_messag
 }
 
 void common_hal_canio_listener_deinit(canio_listener_obj_t *self) {
-    // free our FIFO, clear our matches, SOMETHING
     if (self->can) {
         clear_filters(self);
         if (self->fifo_idx == 0) {
diff --git a/shared-bindings/_canio/CAN.c b/shared-bindings/_canio/CAN.c
index 53045c35a..537aa5580 100644
--- a/shared-bindings/_canio/CAN.c
+++ b/shared-bindings/_canio/CAN.c
@@ -53,8 +53,8 @@
 //|         connected to a transceiver which controls the H and L pins on a shared
 //|         bus.
 //|
-//|         :param ~microcontrller.Pin rx: the pin to receive with.
-//|         :param ~microcontrller.Pin tx: the pin to transmit with, or None if the peripheral should operate in "silent" mode.
+//|         :param ~microcontroller.Pin rx: the pin to receive with.
+//|         :param ~microcontroller.Pin tx: the pin to transmit with, or None if the peripheral should operate in "silent" mode.
 //|         :param int baudrate: The bit rate of the bus in Hz.  All devices on the bus must agree on this value.
 //|         :param bool loopback: True if the peripheral will be operated in loopback mode.
 //|         :param bool auto_restart: If True, will restart communications after entering bus-off state
@@ -76,13 +76,8 @@ STATIC mp_obj_t canio_can_make_new(const mp_obj_type_t *type, size_t n_args, con
 
     mp_arg_parse_all(n_args, pos_args, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
 
-mp_printf(&mp_plat_print, "ARG_rx=%d args[ARG_rx].u_obj=%p\n", ARG_rx, args[ARG_rx].u_obj);
-mp_printf(&mp_plat_print, "ARG_tx=%d args[ARG_tx].u_obj=%p\n", ARG_tx, args[ARG_tx].u_obj);
-
     mcu_pin_obj_t *rx_pin = validate_obj_is_free_pin(args[ARG_rx].u_obj);
-mp_printf(&mp_plat_print, "rx_pin=%p\n", rx_pin);
     mcu_pin_obj_t *tx_pin = validate_obj_is_free_pin_or_none(args[ARG_tx].u_obj);
-mp_printf(&mp_plat_print, "tx_pin=%p\n", tx_pin);
 
     canio_can_obj_t *self = m_new_obj(canio_can_obj_t);
     self->base.type = &canio_can_type;
@@ -94,7 +89,7 @@ mp_printf(&mp_plat_print, "tx_pin=%p\n", tx_pin);
 }
 
 
-//|     auto_restart: int
+//|     auto_restart: bool
 //|     """If True, will restart communications after entering bus-off state"""
 //|
 STATIC mp_obj_t canio_can_auto_restart_get(mp_obj_t self_in) {
@@ -240,12 +235,6 @@ STATIC const mp_obj_property_t canio_can_state_obj = {
 };
 
 
-#if 0
-//|     # pending_tx_count: int
-//|     # """The number of messages waiting to be transmitted. (read-only)"""
-//|
-#endif
-
 //|     def restart(self) -> None:
 //|         """If the device is in the bus off state, restart it."""
 //|         ...
diff --git a/shared-bindings/_canio/Listener.c b/shared-bindings/_canio/Listener.c
index a3890f8dd..84b2aacb9 100644
--- a/shared-bindings/_canio/Listener.c
+++ b/shared-bindings/_canio/Listener.c
@@ -39,7 +39,7 @@
 //|
 
 //|     def read(self) -> Optional[Message]:
-//|         """Returns a message, after waiting up to self.timeout seconds
+//|         """Reads a message, after waiting up to self.timeout seconds
 //|
 //|         If no message is received in time, None is returned.  Otherwise,
 //|         a Message is returned."""
@@ -62,10 +62,8 @@ STATIC mp_obj_t canio_listener_read(mp_obj_t self_in) {
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(canio_listener_read_obj, canio_listener_read);
 
 //|     def readinto(self, message: Message) -> bool:
-//|         """Returns a message, after waiting up to self.timeout seconds
-//|
-//|         Returns True (and modifies message) if a message was received,
-//|         False otherwise."""
+//|         """Returns True (and modifies message) if a message was received
+//|         before ``timeout`` seconds elapsed, False otherwise."""
 //|         ...
 //|
 STATIC mp_obj_t canio_listener_readinto(mp_obj_t self_in, mp_obj_t message) {
@@ -115,7 +113,7 @@ STATIC mp_obj_t canio_listener_next(mp_obj_t self_in) {
     if (common_hal_canio_listener_in_waiting(self)) {
         return canio_listener_read(self_in);
     }
-    return self;
+    return MP_OBJ_STOP_ITERATION;
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(canio_listener_next_obj, canio_listener_next);
 
diff --git a/shared-bindings/_canio/Match.c b/shared-bindings/_canio/Match.c
index b71529b73..4ff35906b 100644
--- a/shared-bindings/_canio/Match.c
+++ b/shared-bindings/_canio/Match.c
@@ -25,7 +25,6 @@
  */
 
 #include "shared-bindings/_canio/Match.h"
-#include "shared-module/_canio/Match.h"
 
 #include "py/objproperty.h"
 #include "py/runtime.h"
@@ -46,7 +45,7 @@
 STATIC mp_obj_t canio_match_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     enum { ARG_address, ARG_mask, ARG_extended, NUM_ARGS };
     static const mp_arg_t allowed_args[] = {
-        { MP_QSTR_address, MP_ARG_INT | MP_ARG_REQUIRED, {.u_int = 0} },
+        { MP_QSTR_address, MP_ARG_INT | MP_ARG_REQUIRED },
         { MP_QSTR_mask, MP_ARG_INT, {.u_int = 0} },
         { MP_QSTR_extended, MP_ARG_BOOL, {.u_bool = false} },
     };
@@ -79,7 +78,7 @@ STATIC mp_obj_t canio_match_make_new(const mp_obj_type_t *type, size_t n_args, c
 
 STATIC mp_obj_t canio_match_address_get(mp_obj_t self_in) {
     canio_match_obj_t *self = self_in;
-    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_match_address_get(self));
+    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_match_get_address(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_match_address_get_obj, canio_match_address_get);
 
@@ -97,7 +96,7 @@ const mp_obj_property_t canio_match_address_obj = {
 
 STATIC mp_obj_t canio_match_mask_get(mp_obj_t self_in) {
     canio_match_obj_t *self = self_in;
-    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_match_mask_get(self));
+    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_match_get_mask(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_match_mask_get_obj, canio_match_mask_get);
 
@@ -114,7 +113,7 @@ const mp_obj_property_t canio_match_mask_obj = {
 
 STATIC mp_obj_t canio_match_extended_get(mp_obj_t self_in) {
     canio_match_obj_t *self = self_in;
-    return mp_obj_new_bool(common_hal_canio_match_extended_get(self));
+    return mp_obj_new_bool(common_hal_canio_match_get_extended(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_match_extended_get_obj, canio_match_extended_get);
 
diff --git a/shared-bindings/_canio/Match.h b/shared-bindings/_canio/Match.h
index adf98eb86..5522c2636 100644
--- a/shared-bindings/_canio/Match.h
+++ b/shared-bindings/_canio/Match.h
@@ -27,7 +27,11 @@
 #pragma once
 
 #include "py/obj.h"
+#include "shared-module/_canio/Match.h"
 
 extern const mp_obj_type_t canio_match_type;
 
-// Nothing now.
+void common_hal_canio_match_construct(canio_match_obj_t *self, int address, int mask, bool extended);
+int common_hal_canio_match_get_address(const canio_match_obj_t *self);
+int common_hal_canio_match_get_mask(const canio_match_obj_t *self);
+bool common_hal_canio_match_get_extended(const canio_match_obj_t *self);
diff --git a/shared-bindings/_canio/Message.c b/shared-bindings/_canio/Message.c
index bfaaf52b2..85657e05e 100644
--- a/shared-bindings/_canio/Message.c
+++ b/shared-bindings/_canio/Message.c
@@ -25,7 +25,6 @@
  */
 
 #include "shared-bindings/_canio/Message.h"
-#include "shared-module/_canio/Message.h"
 
 #include "py/obj.h"
 #include "py/objproperty.h"
@@ -33,7 +32,7 @@
 
 //| class Message:
 //|     def __init__(self, id: int=0, data: Optional[bytes] = None, *, size: Optional[int] = None, rtr: bool = False, extended: bool = False):
-//|         """Construct a Message to send on a CAN bus
+//|         """Construct a Message to use with a CAN bus.  Provide arguments to create a message to send.  Otherwise, use Listener.readinto() to read a message.
 //|
 //|         :param int id: The numeric ID of the message
 //|         :param bytes data: The content of the message
@@ -97,13 +96,13 @@ STATIC mp_obj_t canio_message_make_new(const mp_obj_type_t *type, size_t n_args,
 //|
 STATIC mp_obj_t canio_message_id_get(const mp_obj_t self_in) {
     canio_message_obj_t *self = self_in;
-    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_message_id_get(self));
+    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_message_get_id(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_message_id_get_obj, canio_message_id_get);
 
 STATIC mp_obj_t canio_message_id_set(const mp_obj_t self_in, const mp_obj_t id) {
     canio_message_obj_t *self = self_in;
-    common_hal_canio_message_id_set(self, mp_obj_get_int(id));
+    common_hal_canio_message_set_id(self, mp_obj_get_int(id));
     return mp_const_none;
 }
 MP_DEFINE_CONST_FUN_OBJ_2(canio_message_id_set_obj, canio_message_id_set);
@@ -122,7 +121,7 @@ STATIC const mp_obj_property_t canio_message_id_obj = {
 //|
 STATIC mp_obj_t canio_message_data_get(const mp_obj_t self_in) {
     canio_message_obj_t *self = self_in;
-    return mp_obj_new_bytes((const byte*)common_hal_canio_message_data_get(self), common_hal_canio_message_size_get(self));
+    return mp_obj_new_bytes((const byte*)common_hal_canio_message_get_data(self), common_hal_canio_message_get_size(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_message_data_get_obj, canio_message_data_get);
 
@@ -133,7 +132,7 @@ STATIC mp_obj_t canio_message_data_set(const mp_obj_t self_in, const mp_obj_t da
     if (data.len > 8) {
         mp_raise_ValueError(translate("Messages limited to 8 bytes"));
     }
-    common_hal_canio_message_data_set(self, data.buf, data.len);
+    common_hal_canio_message_set_data(self, data.buf, data.len);
     return mp_const_none;
 }
 MP_DEFINE_CONST_FUN_OBJ_2(canio_message_data_set_obj, canio_message_data_set);
@@ -154,7 +153,7 @@ STATIC const mp_obj_property_t canio_message_data_obj = {
 //|
 STATIC mp_obj_t canio_message_size_get(const mp_obj_t self_in) {
     canio_message_obj_t *self = self_in;
-    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_message_size_get(self));
+    return MP_OBJ_NEW_SMALL_INT(common_hal_canio_message_get_size(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_message_size_get_obj, canio_message_size_get);
 
@@ -164,7 +163,7 @@ STATIC mp_obj_t canio_message_size_set(const mp_obj_t self_in, const mp_obj_t si
     if (size > 8) {
         mp_raise_ValueError(translate("Messages limited to 8 bytes"));
     }
-    common_hal_canio_message_size_set(self, size);
+    common_hal_canio_message_set_size(self, size);
     return mp_const_none;
 }
 MP_DEFINE_CONST_FUN_OBJ_2(canio_message_size_set_obj, canio_message_size_set);
@@ -182,13 +181,13 @@ STATIC const mp_obj_property_t canio_message_size_obj = {
 //|
 STATIC mp_obj_t canio_message_extended_get(const mp_obj_t self_in) {
     canio_message_obj_t *self = self_in;
-    return mp_obj_new_bool(common_hal_canio_message_extended_get(self));
+    return mp_obj_new_bool(common_hal_canio_message_get_extended(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_message_extended_get_obj, canio_message_extended_get);
 
 STATIC mp_obj_t canio_message_extended_set(const mp_obj_t self_in, const mp_obj_t extended) {
     canio_message_obj_t *self = self_in;
-    common_hal_canio_message_size_set(self, mp_obj_is_true(extended));
+    common_hal_canio_message_set_extended(self, mp_obj_is_true(extended));
     return mp_const_none;
 }
 MP_DEFINE_CONST_FUN_OBJ_2(canio_message_extended_set_obj, canio_message_extended_set);
@@ -207,13 +206,13 @@ STATIC const mp_obj_property_t canio_message_extended_obj = {
 //|
 STATIC mp_obj_t canio_message_rtr_get(const mp_obj_t self_in) {
     canio_message_obj_t *self = self_in;
-    return mp_obj_new_bool(common_hal_canio_message_rtr_get(self));
+    return mp_obj_new_bool(common_hal_canio_message_get_rtr(self));
 }
 MP_DEFINE_CONST_FUN_OBJ_1(canio_message_rtr_get_obj, canio_message_rtr_get);
 
 STATIC mp_obj_t canio_message_rtr_set(const mp_obj_t self_in, const mp_obj_t rtr) {
     canio_message_obj_t *self = self_in;
-    common_hal_canio_message_size_set(self, mp_obj_is_true(rtr));
+    common_hal_canio_message_set_rtr(self, mp_obj_is_true(rtr));
     return mp_const_none;
 }
 MP_DEFINE_CONST_FUN_OBJ_2(canio_message_rtr_set_obj, canio_message_rtr_set);
diff --git a/shared-bindings/_canio/Message.h b/shared-bindings/_canio/Message.h
index 32d6801bc..ed739af0d 100644
--- a/shared-bindings/_canio/Message.h
+++ b/shared-bindings/_canio/Message.h
@@ -27,5 +27,18 @@
 #pragma once
 
 #include "py/obj.h"
+#include "shared-module/_canio/Message.h"
 
 extern const mp_obj_type_t canio_message_type;
+
+void common_hal_canio_message_construct(canio_message_obj_t *self, int id, void *data, size_t size, bool rtr, bool extended);
+const void *common_hal_canio_message_get_data(const canio_message_obj_t *self);
+void common_hal_canio_message_set_data(canio_message_obj_t *self, const void *data, size_t size);
+bool common_hal_canio_message_get_extended(const canio_message_obj_t *self);
+void common_hal_canio_message_set_extended(canio_message_obj_t *self, bool extended);
+int common_hal_canio_message_get_id(const canio_message_obj_t *self);
+void common_hal_canio_message_set_id(canio_message_obj_t *self, int id);
+bool common_hal_canio_message_get_rtr(const canio_message_obj_t *self);
+void common_hal_canio_message_set_rtr(canio_message_obj_t *self, bool rtr);
+size_t common_hal_canio_message_get_size(const canio_message_obj_t *self);
+void common_hal_canio_message_set_size(canio_message_obj_t *self, size_t size);
diff --git a/shared-module/_canio/Match.c b/shared-module/_canio/Match.c
index 2b696addb..3dcde18a7 100644
--- a/shared-module/_canio/Match.c
+++ b/shared-module/_canio/Match.c
@@ -32,12 +32,12 @@ void common_hal_canio_match_construct(canio_match_obj_t *self, int address, int
     self->extended = extended;
 }
 
-int common_hal_canio_match_address_get(const canio_match_obj_t *self) {
+int common_hal_canio_match_get_address(const canio_match_obj_t *self) {
     return self->address;
 }
-int common_hal_canio_match_mask_get(const canio_match_obj_t *self) {
+int common_hal_canio_match_get_mask(const canio_match_obj_t *self) {
     return self->mask;
 }
-bool common_hal_canio_match_extended_get(const canio_match_obj_t *self) {
+bool common_hal_canio_match_get_extended(const canio_match_obj_t *self) {
     return self->extended;
 }
diff --git a/shared-module/_canio/Match.h b/shared-module/_canio/Match.h
index 099df976d..25f047f37 100644
--- a/shared-module/_canio/Match.h
+++ b/shared-module/_canio/Match.h
@@ -34,8 +34,3 @@ typedef struct {
     int mask;
     bool extended;
 } canio_match_obj_t;
-
-void common_hal_canio_match_construct(canio_match_obj_t *self, int address, int mask, bool extended);
-int common_hal_canio_match_address_get(const canio_match_obj_t *self);
-int common_hal_canio_match_mask_get(const canio_match_obj_t *self);
-bool common_hal_canio_match_extended_get(const canio_match_obj_t *self);
diff --git a/shared-module/_canio/Message.c b/shared-module/_canio/Message.c
index c0403b519..0caca58f0 100644
--- a/shared-module/_canio/Message.c
+++ b/shared-module/_canio/Message.c
@@ -41,57 +41,57 @@ void common_hal_canio_message_construct(canio_message_obj_t *self, int id, void
     }
 }
 
-int common_hal_canio_message_id_get(const canio_message_obj_t *self)
+int common_hal_canio_message_get_id(const canio_message_obj_t *self)
 {
     return self->id;
 }
 
-void common_hal_canio_message_id_set(canio_message_obj_t *self, int id)
+void common_hal_canio_message_set_id(canio_message_obj_t *self, int id)
 {
     self->id = id;
 }
 
 
-const void *common_hal_canio_message_data_get(const canio_message_obj_t *self)
+const void *common_hal_canio_message_get_data(const canio_message_obj_t *self)
 {
     return self->data;
 }
 
-const void common_hal_canio_message_data_set(canio_message_obj_t *self, const void *data, size_t size)
+const void common_hal_canio_message_set_data(canio_message_obj_t *self, const void *data, size_t size)
 {
     self->size = size;
     memcpy(self->data, data, size);
 }
 
 
-size_t common_hal_canio_message_size_get(const canio_message_obj_t *self)
+size_t common_hal_canio_message_get_size(const canio_message_obj_t *self)
 {
     return self->size;
 }
 
-void common_hal_canio_message_size_set(canio_message_obj_t *self, size_t size)
+void common_hal_canio_message_set_size(canio_message_obj_t *self, size_t size)
 {
     memset(self->data, 0, size);
     self->size = size;
 }
 
 
-bool common_hal_canio_message_rtr_get(const canio_message_obj_t *self)
+bool common_hal_canio_message_get_rtr(const canio_message_obj_t *self)
 {
     return self->rtr;
 }
 
-void common_hal_canio_message_rtr_set(canio_message_obj_t *self, bool rtr)
+void common_hal_canio_message_set_rtr(canio_message_obj_t *self, bool rtr)
 {
     self->rtr = rtr;
 }
 
-bool common_hal_canio_message_extended_get(const canio_message_obj_t *self)
+bool common_hal_canio_message_get_extended(const canio_message_obj_t *self)
 {
     return self->extended;
 }
 
-void common_hal_canio_message_extended_set(canio_message_obj_t *self, bool extended)
+void common_hal_canio_message_set_extended(canio_message_obj_t *self, bool extended)
 {
     self->extended = extended;
 }
diff --git a/shared-module/_canio/Message.h b/shared-module/_canio/Message.h
index b9fd8eaf2..b99c5c48e 100644
--- a/shared-module/_canio/Message.h
+++ b/shared-module/_canio/Message.h
@@ -36,15 +36,3 @@ typedef struct {
     bool rtr:1;
     bool extended:1;
 } canio_message_obj_t;
-
-void common_hal_canio_message_construct(canio_message_obj_t *self, int id, void *data, size_t size, bool rtr, bool extended);
-bool common_hal_canio_message_rtr_get(const canio_message_obj_t *self);
-bool common_hal_canio_message_extended_get(const canio_message_obj_t *self);
-int common_hal_canio_message_id_get(const canio_message_obj_t *self);
-const void *common_hal_canio_message_data_get(const canio_message_obj_t *self);
-size_t common_hal_canio_message_size_get(const canio_message_obj_t *self);
-void common_hal_canio_message_rtr_set(canio_message_obj_t *self, bool rtr);
-void common_hal_canio_message_extended_set(canio_message_obj_t *self, bool extended);
-void common_hal_canio_message_id_set(canio_message_obj_t *self, int id);
-void common_hal_canio_message_data_set(canio_message_obj_t *self, const void *data, size_t size);
-void common_hal_canio_message_size_set(canio_message_obj_t *self, size_t size);
-- 
cgit v1.2.3