On 4/5/19 3:30 PM, David Jander wrote:
> 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.
As I said in the other mail: A j1939 message is identified by sa/da/pgn.
If one of these is different it's a different message.
> 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".
Yes.
If you use a different DA, the message will be received by someone else.
(However I can image that Engine RPM would be broadcasted.) Or if you
use a different SA, you impersonate someone else :) And if you change
the PGN, it's not Engine RPM anymore.
> If I bind() to a different PGN, that would be the source PGN
...to be precise, it's the PGN the sender of the message used.
> 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.
...yes. You both have convinced me several mails ago, that we're going
to rename back to "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().
ACK. It will be the pgn from connect(), however the pgn of sendto() will
overwrite this.
> I hope this explanation is not too confusing.
:D
>>>> 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".
ACK.
> 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.
ACK - the UAPI is described correct. The kernel space will use "pgn" only :)
> But I agree with Kurt that probably just "pgn" is a better name, specially
> someone well versed in J1939.
ACK
> Did I make the confusion worse now?
No. It's pgn by 4:0 consensus.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
