Message-ID: <Zh6m3jkRovDutKnZ@fedora>
Date: Tue, 16 Apr 2024 18:27:10 +0200
From: Francesco Valla <valla.francesco@...il.com>
To: Vincent Mailhol <vincent.mailhol@...il.com>
Cc: Oliver Hartkopp <socketcan@...tkopp.net>,
	Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Simon Horman <horms@...nel.org>,
	Bagas Sanjaya <bagasdotme@...il.com>, fabio@...aril.me
Subject: Re: [PATCH v2 1/1] Documentation: networking: document ISO
 15765-2:2016

Hi Vincent,

thank you for the review!

I'll omit from this reply the issue about the standard to be referenced
and the CAN-CC naming (discussed in another thread also with Oliver).

About the typos and formatting observations: rst is not my native
language (I'm more on the Markdown side), I'll apply all the corrections
you suggested. Thank you.

Some other considerations follow.

On Sat, Apr 13, 2024 at 10:11:55PM +0900, Vincent Mailhol wrote:
> Hi Francesco,
> 
> Thank you for the ISO-TP documentation.
> 
> I left a few comments, but overall, good work. Also, I did not double
> check each individual option one by one.

(...)

> > +
> > +- physical addressing is implemented by two node-specific addresses (CAN
> > +  identifiers) and is used in 1-to-1 communication
> > +- functional addressing is implemented by one node-specific address (CAN
> > +  identifier) and is used in 1-to-N communication
> > +
> > +In a so-called "normal" addressing scenario, both these addresses are
> > +represented by a 29-bit CAN ID. However, in order to support larger networks,
> > +an "extended" addressing scheme can be adopted: in this case, the first byte of
> > +the data payload is used as an additional component of the address (both for
> > +the physical and functional cases); two different CAN IDs are still required.
> 
> There is more than that.
> 
>   - The normal addressing can also use the non-extended 11 bits CAN ID.
>   - In addition to the normal and extended addressing mode, there
>     is a third mode: the mixed addressing.
> 
> Ref:
> 
>   - ISO 15765:2024 §10.3.1 "Addressing formats"
>   - https://www.embedded-communication.com/en/misc/iso-tp-addressing/
> 

You are right. I'll drop the reference to "29-bit" and add the mixed
addressing (I did not know it, I'll have to investigate a bit - I
personally always used the normal one).

(...)

> 
> > +Unlike the CAN_RAW socket API, only the data payload shall be specified in all
> > +these calls, as the CAN header is automatically filled by the ISO-TP stack
> > +using information supplied during socket creation. In the same way, the stack
> 
> This is making a shortcut. There are the raw CAN payload and the
> ISO-TP payload. In this paragraph it is not clear that "data payload"
> is referring to the ISO-TP payload.
> 
> Also, what is the meaning of "the CAN header". Here, I think you mean
> CAN ID plus some of the few first byte of the CAN payload.
> 
> I suggest that you use more precise vocabulary from the standard:
> 
>   - Address information
>   - Protocol Information
>   - Data field
> 
> Something like:
> 
>   only the ISO-TP data field (the actual payload) is sent. The
>   address information and the protocol information is
>   automatically filled by the ISO-TP stack...
>

Indeed it is a shortcut. Your suggestion to adhere more to the standard
is welcome, I'd rephrase as:

  Unlike the CAN_RAW socket API, only the ISO-TP data field (the actual payload)
  is sent and received by the userspace application using these calls. The address
  information and the protocol information are automatically filled by the ISO-TP
  stack using the configuration supplied during socket creation. In the same way,
  the stack will use the transport mechanism when required (i.e., when the size
  of the data payload exceeds the MTU of the underlying CAN bus).


(...)

> > +Examples
> > +========
> > +
> > +Basic node example
> > +------------------
> > +
> > +Following example implements a node using "normal" physical addressing, with
> > +RX ID equal to 0x18DAF142 and a TX ID equal to 0x18DA42F1. All options are left
> > +to their default.
> > +
> > +.. code-block:: C
> > +
> > +  int s;
> > +  struct sockaddr_can addr;
> 
> Here, I would suggest the C99 designated field initialization:
> 
>   struct sockaddr_can addr = {
>           .can_family = AF_CAN;
>           .can_ifindex = if_nametoindex("can0");
>           .tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
>           .tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
>   };
> 
> Well, this is just a suggestion, feel free to reject it if you do not like it.
> 

Not a fan of C99 designated field initialization inside functions, TBH.
Moreover, these parameters are typically specified through either the
command line or some configuration file. I'll keep my version.

> > +  int ret;
> > +
> > +  s = socket(PF_CAN, SOCK_DGRAM, CAN_ISOTP);
> > +  if (s < 0)
> > +      exit(1);
> > +
> > +  addr.can_family = AF_CAN;
> > +  addr.can_ifindex = if_nametoindex("can0");
> 
> if_nametoindex() may fail. Because you are doing error handling in
> this example, do it also here:
> 
>   if (!addr.can_ifindex)
>           err("if_nametoindex()");
> 
> > +  addr.tp.tx_id = (0x18DA42F1 | CAN_EFF_FLAG);
> > +  addr.tp.rx_id = (0x18DAF142 | CAN_EFF_FLAG);
> 
> Nitpick: the bracket are not needed here:
> 
>   addr.tp.tx_id = 0x18DA42F1 | CAN_EFF_FLAG;
>   addr.tp.rx_id = 0x18DAF142 | CAN_EFF_FLAG;
>

Ack.

> > +
> > +  ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
> > +  if (ret < 0)
> > +      exit(1);
> > +
> > +  // Data can now be received using read(s, ...) and sent using write(s, ...)
> 
> Kernel style prefers C style comments over C++. I think that should
> also apply to the documentation:
> 
>   /* Data can now be received using read(s, ...) and sent using write(s, ...) */
> 

Ack.


Again, thank you for the review.

Regards,
Francesco

Powered by blists - more mailing lists