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

Quick Start (README.rst) seems not quite correct #54

Open
grant-allan-ctct opened this issue Nov 30, 2023 · 1 comment
Open

Quick Start (README.rst) seems not quite correct #54

grant-allan-ctct opened this issue Nov 30, 2023 · 1 comment

Comments

@grant-allan-ctct
Copy link
Collaborator

Hello, I am starting out using this library, so looking at the introductory documentation. There are a couple of things that I noticed.

  1. Between the simple example and the more advanced example, there is a line of text introducing the advance example, stating:

A more sophisticated example in which the CA class was overloaded to include its own functionality:

This mention of the CA class being "overloaded" looks like a leftover from the previous advanced example that existed in the Frank Benkert repo that this current project was forked from. Further than that, the author in Frank's repo seems to have confused the words "overloaded" and "overridden". His repo's advanced example derives a child class from the j1939.ControllerApplication class, and in each function of his child class where a function override is provided, there appears the words (OVERLOADED function).

I think that we should remove the sentence about the CA class being overloaded. It's even less true now that it was in Frank's day. 😎

  1. In the advanced example, there are two callback functions named ca_timer_callback1 and ca_timer_callback2. Both of them contain the comment:
    This callback is registered at the ECU timer event mechanism to be
    executed every 500ms.

But when you go and look at the code in main, you can see that one callback is set up with a period of 0.5 seconds and the other callback is set up with a period of 5 seconds. (In my opinion, it would be better decoupling of main() and the callback functions if the commetary in the callback functions did not mention how often they are being called. This would be better "Separation of Concerns", in my opinion. Ideally, the callback should not know/care how often it will be invoked. Advantage: we would no longer need to make changes in two distinct locations if, in the future, there was a desire to change one of the frequencies.)

I might be able to find some time this weekend to have a look and make a PR, if there is interest.

@grant-allan-ctct
Copy link
Collaborator Author

This comment in the advanced sample's ca_receive function seems also to be a leftover from the historical advanced sample (visible in https://github.com/benkfra/j1939).

        def ca_receive(priority, pgn, source, timestamp, data):
            """Feed incoming message to this CA.
            (OVERLOADED function)

Unless I am mistaken, this ca_receive function is just a plain callback. Contrary to the comment block, the caller of this function is not feeding anything to the CA. Further, it's not an "OVERLOADED" (nor an over-ridden) function.

As this is "Getting started" documentation, I think that it's in the best interest of newcomers (and therefore the project), to remove the items of misleading information, which (in my experience) represent barriers to quickly gaining an understanding of the project and how to use it.

I have taken a fork of the project, and once I gain some more confidence in my learning, I might be in a position to propose some changes in a PR (but not yet 😎).

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