On Fri, 5 Apr 2019 15:07:44 +0200
Kurt Van Dijck <dev.kurt@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On vr, 05 apr 2019 14:07:43 +0200, Marc Kleine-Budde wrote:
> > On 4/5/19 11:46 AM, Kurt Van Dijck wrote:
> > > On vr, 05 apr 2019 08:33:52 +0200, David Jander wrote:
> > >> On Thu, 4 Apr 2019 19:54:30 +0200
> > >> Kurt Van Dijck <dev.kurt@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > >>
> > >>> On vr, 29 mrt 2019 14:58:38 +0100, Oleksij Rempel wrote:
> > >>>> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> > >>>> index df058d08fe68..127b4e28b16a 100644
> > >>>> --- a/net/can/j1939/j1939-priv.h
> > >>>> +++ b/net/can/j1939/j1939-priv.h
> > >>>> @@ -113,7 +113,7 @@ struct j1939_ecu *j1939_ecu_get_by_name_locked(struct j1939_priv *priv,
> > >>>> struct j1939_addr {
> > >>>> name_t src_name;
> > >>>> name_t dst_name;
> > >>>> - pgn_t pgn;
> > >>>> + pgn_t dst_pgn;
> > >>>>
> > >>>> u8 sa;
> > >>>> u8 da;
> > >>>
> > >>> I considered j1939_addr like a kind of label of a packet. In j1939, a
> > >>> packet has only 1 pgn.
> > >>> I'm curious why you need 2.
> > >>
> > >> There is only one. It just was renamed from pgn to dst_pgn, probably to make
> > >> clear that it is related to the destination and not the source (which wouldn't
> > >> make any sense).
> > >
> > > Well, the PGN describes the packet that the source sends to the destination,
> > > so it's equally related to the source as to the destination.
> > > renaming it didn't clarify for me, in contrary.
> > >
> > > I suppose it's nitpicking, so if you all think it's better
> > > understandable like that, then I'm ok too.
> > > It's not visible in userspace anyway.
> >
> > ...but the kernel code should be readable, too.
> that is true also :-)
> >
> > As said in the other mail the dst_pgn is the pgn label on the packets.
> > The "other" pgn, which is named pgn_rx_filter and member in the struct
> > j1939_sock, is set by bind() and only used to filter incoming packets.
>
> Oleksij created the right variables and the right code.
> Only the name 'dst_pgn' confuses me.
>
> The Engine RPM PGN, for example, is name named to its contents, and
> source, not destination. It's PGN number should not be called 'dst_pgn'.
> Any broadcasted PGN qualifies for this example, and non-broadcasted
> PGN's do not inherently justify the opposite.
Personally I agree with Kurt, but I can understand the confusion of J1939
non-experts in the sense that if the PGN is part of an address (in this case),
this is the PGN used for sending, and thus "destination" is in the name.
For something such as "Engine RPM", the way of thinking is: If I want to
transmit the RPM value, to someone (who bind()ed to the Engine RPM PGN), I need
to set the PGN as part of the "destination". If I bind() to a different PGN,
that would be the source PGN and all messages I receive have destination NAME
and address set to my own NAME and address (I am the destination) and so also
the destination PGN.
But if I transmit Engine RPM, then I need to set the destination PGN, and it
will NOT be the PGN I used for bind(). I hope this explanation is not too
confusing.
> > > so it's equally related to the source as to the destination.
>
> It should neither be called 'src_pgn', just plain 'pgn'.
Depends on how you view it and what is more intuitive to someone who needs to
read the code. In the case of broadcast PGNs like Engine RPM this might not be
as clear as for example the communication between a file-server and a
file-client. The server "listens" on the PGN "A" and sends responses to the
PGN "B". So if the client wants to send a message to the server, it will
bind() to his NAME and to PGN "B", and then connect() to the server NAME and
PGN "A". After that it just calls send() and recv() to communicate. When
calling send(), the PGN set with connect() gets filled into the message, and
since semantically the connect() is to the destination of the message, one
could call it dst_pgn.
But I agree with Kurt that probably just "pgn" is a better name, specially
someone well versed in J1939.
Did I make the confusion worse now?
Best regards,
--
David Jander
Protonic Holland.
