Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscribers are not notified for certain PGNs #57

Open
grant-allan-ctct opened this issue Dec 3, 2023 · 2 comments
Open

Subscribers are not notified for certain PGNs #57

grant-allan-ctct opened this issue Dec 3, 2023 · 2 comments

Comments

@grant-allan-ctct
Copy link
Collaborator

In the sample code in README.rst, we see a statement here that says

To simply receive all passing (public) messages on the bus you can subscribe to the ECU object.

But it turns out that this is not true. Certain PGNs will be diverted elsewhere and not make it as far as the subscribers, as can be seen here. The following items will not get to the subscribers:

  • ParameterGroupNumber.PGN.ADDRESSCLAIM
  • ParameterGroupNumber.PGN.REQUEST
  • ParameterGroupNumber.PGN.TP_CM
  • ParameterGroupNumber.PGN.DATATRANSFER

If we want it to be a true statement that all passing message will be received by subscribers, then I think that we must remove the else: on line 521 of j1939_21.py and unindent the following lines.

@grant-allan-ctct
Copy link
Collaborator Author

grant-allan-ctct commented Dec 4, 2023

Here is a way to use subclassing in order to receive notifications of (a selection of) the missing PGNs without changing any python-can-j1939 code.

It's a bit icky, but it might be useful to someone, depending on what practical situation they might be in. Two of the j1939 classes are subclassed in this workaround, ElectronicControlUnit and J1939_21. These imports are needed:

from j1939 import ElectronicControlUnit
from j1939.parameter_group_number import ParameterGroupNumber
from j1939.message_id import MessageId
from j1939.j1939_21 import J1939_21

Here is a customized J1939_21 class that can notify any of the "missing" PGNs after they have had their usual processing. When creating an instance of this class, pass in a notify_these_also named argument that lists whichever missing PGNs are wanted:

class CustomJ1939_21(J1939_21):

    def __init__(self, *args, **kwargs):
        self.__notifyTheseAlso = kwargs.get("notify_these_also")
        self.__notifyCallback = args[2] # _notify_subscribers
        super().__init__(*args)
        
    def notify(self, can_id, data, timestamp):
        super().notify(can_id, data, timestamp)
        # now notify any received messages that we're interested in, but which the regular J1939_21 class omits:
        mid = MessageId(can_id=can_id)
        pgn = ParameterGroupNumber()
        pgn.from_message_id(mid)
        pgn_value = pgn.value & 0x1FF00
        if pgn_value in self.__notifyTheseAlso:
            dest_address = pgn.pdu_specific
            self.__notifyCallback(mid.priority, pgn_value, mid.source_address, dest_address, timestamp, data)

The below change is circumventing the way that the ElectronicControlUnit constructor wants to create either a J1939_21 or a J1939_22 instance. In order to substitute the above subclass of J1939_21 instead, we are needing to get rid of the self.j1939_dll (data link layer) object that got created during ElectronicControlUnit construction, and replace it with our custom one - and we want to achieve this before the async job starts looping:

class CustomECU(ElectronicControlUnit):

    def __init__(self, **kwargs):
        self.max_cmdt_packets = kwargs.get('max_cmdt_packets', 1)
        self.minimum_tp_rts_cts_dt_interval = kwargs.get('minimum_tp_rts_cts_dt_interval', None)
        self.minimum_tp_bam_dt_interval = kwargs.get('minimum_tp_bam_dt_interval', None)
        super().__init__(**kwargs)

    def _async_job_thread(self):
        del self.j1939_dll
        self.j1939_dll = CustomJ1939_21(self.send_message, 
                                        self._job_thread_wakeup, 
                                        self._notify_subscribers, 
                                        self.max_cmdt_packets, 
                                        self.minimum_tp_rts_cts_dt_interval, 
                                        self.minimum_tp_bam_dt_interval, 
                                        self._is_message_acceptable,
                                        notify_these_also = [
                                            ParameterGroupNumber.PGN.ADDRESSCLAIM,
                                            ParameterGroupNumber.PGN.REQUEST,
                                            ParameterGroupNumber.PGN.TP_CM,
                                            ParameterGroupNumber.PGN.DATATRANSFER
                                            ]
                                        )
        super()._async_job_thread()

In the notify_these_also list in the above code, I have put all four of the "missing" PGNs, but this can be changed to suit. Maybe the most problematic is the PGN.REQUEST because this PGN is not only used for requesting address claim messages, is it? E.g. it is legit to use this PGN to ask devices on the bus for Software ID (0xFEDA) responses, etc., too?

@grant-allan-ctct
Copy link
Collaborator Author

Addendum to my last note about the non-address-claim PGN.REQUEST not making it to subscribers:

If using the ECU without a CA (the technique mentioned on this line), it seems that the PGN.REQUEST won't get to subscribers, but if using a CA, then the J1939_21.notify function will on this line call the CA's _process_request function, and then the subscriber will be called here. (This applies to every requested PGN except for PGN.ADDRESSCLAIM, which I don't think will make it subscribers at all unless we change the code or apply a workaround such as the one in my prior comment.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant