Canbus: api changes #3466

jepler · 2020-09-24T20:52:44Z

Thanks to @siddacious for finding various problems and suggesting improvements to the API.

Net doc changes from this PR: https://gist.github.com/jepler/37722372347f6847278a67db3cc718c5

Revised loopback test program, passes on SAM E54: https://gist.github.com/d8aedc6b12615970531f54a2f35b9b41

High level summary:

renamed Listener.read -> receive to match CAN.send
removed Listener.readinto (no zero-allocation CAN reading)
added RemoteTransmissionRequest class (no voodoo switching between rtr and non-rtr message types)
Renamed address to id in Match

jepler · 2020-09-24T22:10:57Z

If the noreturn stuff doesn't work, I'll pull it out. However, it DOES become relevant when I start work on the stm32 CAN implementation, it got here by slight accident.

siddacious · 2020-09-24T22:34:02Z

Just saw this, I'll take a look!

siddacious

Looks good to me; the empty for loops are confusing but I don't think they're hurting anything

siddacious · 2020-09-24T23:32:08Z

ports/atmel-samd/reset.c

@@ -32,6 +32,7 @@
 void reset(void) {
    filesystem_flush();
    NVIC_SystemReset();
+    for (;;) {} // unreachable


Does this and the others do something?

Since e.g., NVIC_SystemReset never returns, this creates an unreachable 2 byte opcode that jumps to itself. However, this allows the compiler to conclude that reset() never returns, as promised. A more correct change would be to actually let gcc understand that NVIC_SystemReset does not return, but I don't know how to do that.

There is a macro NORETURN which expands to __atribute__((noreturn)), which is described on this page:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noreturn-function-attribute

Yes, I should have used the macro instead of the __attribute__ here where I added it:

circuitpython/ports/atmel-samd/reset.h

Line 39 in dfdfbc4

void reset(void) __attribute__((noreturn));

gcc is complaning because of the NEW noreturn attribute; when it looks at the function definition it thinks it detects the function DOES return, because NVIC_SystemReset is not noreturn.

because NVIC_SystemReset is not noreturn.

That is the kind of thing to fix in the asf4 repo, I'd say.

jepler · 2020-09-25T17:40:37Z

I revised this PR to get rid of the NORETURN stuff, now in #3469

tannewt

Thanks! I like the address -> id switch but would like to see Message/RTR object split done completely. Otherwise, it's irregular to everything else in shared-bindings.

tannewt · 2020-09-25T18:33:46Z

shared-bindings/canio/Message.c

@@ -170,36 +144,80 @@ STATIC const mp_obj_property_t canio_message_extended_obj = {
              (mp_obj_t)&mp_const_none_obj},
 };

+//| class RemoteTransmissionRequest:


Please put this in a separate file. It'll be hard to find otherwise.

tannewt · 2020-09-25T18:36:27Z

shared-module/canio/Message.c

 {
    self->id = id;
    self->size = size;
-    self->rtr = rtr;
+    self->rtr = !data;


Please just split Message and RTR fully. It's easier understand a consistent structure.

tannewt · 2020-09-25T18:40:05Z

ports/atmel-samd/common-hal/canio/Listener.h

@@ -47,7 +47,7 @@ typedef struct {
 void common_hal_canio_listener_construct(canio_listener_obj_t *self, canio_can_obj_t *can, size_t nmatch, canio_match_obj_t **matches, float timeout);
 void common_hal_canio_listener_check_for_deinit(canio_listener_obj_t *self);
 void common_hal_canio_listener_deinit(canio_listener_obj_t *self);
-bool common_hal_canio_listener_readinto(canio_listener_obj_t *self, canio_message_obj_t *message);
+bool common_hal_canio_listener_receiveinto(canio_listener_obj_t *self, canio_message_obj_t *message);


No need to have these functions here because they are in shared-bindings/canio/Listener.h.

tannewt · 2020-09-25T18:40:54Z

shared-bindings/canio/Listener.h

+void common_hal_canio_listener_construct(canio_listener_obj_t *self, canio_can_obj_t *can, size_t nmatch, canio_match_obj_t **matches, float timeout);
+void common_hal_canio_listener_check_for_deinit(canio_listener_obj_t *self);
+void common_hal_canio_listener_deinit(canio_listener_obj_t *self);
+bool common_hal_canio_listener_receiveinto(canio_listener_obj_t *self, canio_message_obj_t *message);


I'd just have this as receive and return an mp_obj_t. That way you can have two separate structure for Message and RTR.

That won't work out, because the type of message must be determined by the hardware.

Let me amplify on a hastily written answer.

When calling read() or next(), the software has to commit to taking the next messge that has arrived on this FIFO, regardless of whether it is a RTR or not. This is true of all 3 HW implementations I've looked at (SAM E, STM, and ESP32S2).

Having the common_hal routine read into a single structure which can accommodate either type of message is the simplest way I see to do it. Adding a "peek" routine, a union type, or other complication is a needless complication, compared to having an "rtr" field in the C struct.

This implementation detail is transparent to the user, because the two types become distinct Python types with distinct behaviors (one has a data property, one has a size property). As we've discussed in other contexts, the 8 bytes of storage set aside for the message that have to be allocated regardless of whether it's an RTR are also not going to break the bank, it's still within that single GC heap block.

I can change it if it's important, but I don't think it'll be an improvement. If you do want me to change it, please show me what the common_hal APIs should be called and what their parameter lists should look like so that I deliver something that is satisfactory.

I went ahead and wrote what I think you want. It adds about 300 lines of largely duplicate code, because

now common hal's implementation listener's receive needs two copies of the code, to read the HW registers into two different structures

and an additional common hal send remote transmission request API is now needed, which duplicates copying the new rtr structure into the hw registers

This also amounts to about 300 extra bytes of firmware, but for me it is the duplicated code that is the most objectionable.

e.g., this had to be added, while in the existing function RTR is hard-coded as false instead of as message->rtr:

+void common_hal_canio_can_send_remote_transmission_request(canio_can_obj_t *self, canio_remote_transmission_request_obj_t *remote_transmissi on_request) +{ + maybe_auto_restart(self); + + // We have just one dedicated TX buffer, use it! + canio_can_tx_buffer_t *ent = &self->state->tx_buffer[0]; + + ent->txb0.bit.ESI = false; + ent->txb0.bit.XTD = remote_transmission_request->extended; + ent->txb0.bit.RTR = true; + if (remote_transmission_request->extended) { + ent->txb0.bit.ID = remote_transmission_request->id; + } else { + ent->txb0.bit.ID = remote_transmission_request->id << 18; // short addresses are left-justified + } + + ent->txb1.bit.MM = 0; // "remote_transmission_request marker" + ent->txb1.bit.EFC = 0; // don't store fifo events to event queue + ent->txb1.bit.FDF = 0; // Classic CAN format + ent->txb1.bit.BRS = 0; // No bit rate switching + ent->txb1.bit.DLC = remote_transmission_request->size; + + // TX buffer add request + self->hw->TXBAR.reg = 1; + + // wait 8ms (hard coded for now) for TX to occur + uint64_t deadline = port_get_raw_ticks(NULL) + 8; + while (port_get_raw_ticks(NULL) < deadline && !(self->hw->TXBTO.reg & 1)) { + RUN_BACKGROUND_TASKS; + } +} +

changed/added internal APIs are:

-bool common_hal_canio_listener_receiveinto(canio_listener_obj_t *self, canio_message_obj_t *message); +mp_obj_t common_hal_canio_listener_receive(canio_listener_obj_t *self);

and

-void common_hal_canio_can_send(canio_can_obj_t *self, canio_message_obj_t *message); +void common_hal_canio_can_send_message(canio_can_obj_t *self, canio_message_obj_t *message); +void common_hal_canio_can_send_remote_transmission_request(canio_can_obj_t *self, canio_remote_transmission_request_obj_t *remote_transmission_request);

and

-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); -void common_hal_canio_remote_transmission_request_set_length(canio_message_obj_t *self, size_t length);

The comment "+ ent->txb1.bit.MM = 0; // "remote_transmission_request marker"" is just wrong btw

This prepares for creating a separate RemoteTransmissionRequest class

It reuses most of canio.Message's implementation, and structure

…cture This already begins obscuring things, because now there are two sets of shared-module functions for manipulating the same structure, e.g., common_hal_canio_remote_transmission_request_get_id and common_hal_canio_message_get_id

…s a sequence

0 should actually indicate a "match everything" filter which you otherwise couldn't indicate with any single Match object, and an all-address-bits-set number should indicate a "match single address" filter. Use an optional/default None argument to do the job.

siddacious

I believe the above properties are also read-only; can you clarify that in the docs?

This is the case on the SPI implementations MCP2515 and MCP2518.

@tannewt

thanks @tannewt

tannewt

This looks good to me! Thank you for iterating on this.

jepler · 2020-09-30T01:15:53Z

Conflict fixed and also got a straggling (read-only) change request addressed

tannewt

Looks good! Thank you!

jepler requested review from siddacious and tannewt September 24, 2020 20:52

siddacious previously approved these changes Sep 24, 2020

View reviewed changes

jepler dismissed siddacious’s stale review via 964b7a5 September 25, 2020 16:46

jepler force-pushed the canbus-api-changes branch from dfdfbc4 to 964b7a5 Compare September 25, 2020 16:46

jepler requested a review from siddacious September 25, 2020 17:40

tannewt requested changes Sep 25, 2020

View reviewed changes

jepler requested a review from tannewt September 28, 2020 22:21

jepler added 13 commits September 28, 2020 17:22

canio: doc improvements

8b82c23

Match: address -> id

79ca430

Listerner: read -> receive, drop readinto

48bda58

This prepares for creating a separate RemoteTransmissionRequest class

canio: Split RemoteTransmissionRequest to its own class

8d45be1

It reuses most of canio.Message's implementation, and structure

canio: docs: fix how we refer to 'the listen method'

f4fd5bb

canio: Correct type annotations of CAN.send, Listener.receive

ea2f5b6

canio: Listener: shuffle function declarations around

3e97e9c

canio: CAN.listen: rename argument to 'matches', reflecting that it i…

4f7f1e8

…s a sequence

CAN: Use mp_obj_t insteaed of canio_message_obj_t, get rid of rtr field

f4e36fc

canio: remove stray remote_transmission_request method declaration

04e434a

make translate

809225f

jepler force-pushed the canbus-api-changes branch from 2256258 to 809225f Compare September 28, 2020 22:22

jepler added 2 commits September 28, 2020 19:47

can: RemoteTransmissionRequest: remove duplicate docstrings

979ec3a

more doc improvements

03bd968

siddacious suggested changes Sep 29, 2020

View reviewed changes

jepler added 2 commits September 29, 2020 14:05

Allow the _state_count properties to be unimplemented

1bea099

This is the case on the SPI implementations MCP2515 and MCP2518.

canio: actually drop the _error_count properties

611f81a

thanks @tannewt

canio: doc improvement -- don't needlessly call out deinit behavior

d79f4e2

tannewt previously approved these changes Sep 29, 2020

View reviewed changes

siddacious previously approved these changes Sep 29, 2020

View reviewed changes

jepler added 2 commits September 29, 2020 20:13

canio.CAN: clarify read-only status of several properties

88cbf77

Merge remote-tracking branch 'origin/main' into canbus-api-changes

c129c8f

jepler dismissed stale reviews from siddacious and tannewt via c129c8f September 30, 2020 01:15

tannewt approved these changes Sep 30, 2020

View reviewed changes

tannewt merged commit 2ac2f62 into adafruit:main Sep 30, 2020

jepler deleted the canbus-api-changes branch November 3, 2021 21:09

Canbus: api changes #3466

Canbus: api changes #3466

Uh oh!

Conversation

jepler commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jepler commented Sep 24, 2020

Uh oh!

siddacious commented Sep 24, 2020

Uh oh!

siddacious left a comment

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!

jepler Sep 25, 2020 • 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!

jepler commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannewt left a comment

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!

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!

siddacious left a comment

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

jepler commented Sep 30, 2020

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jepler commented Sep 24, 2020 •

edited

Loading

jepler Sep 25, 2020 •

edited

Loading

jepler commented Sep 25, 2020 •

edited

Loading