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.

Sign up for GitHub

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

Jump to bottom

doc: add operational flow figure for netdev #5437

Merged
merged 2 commits into from
Jun 16, 2016
Merged

doc: add operational flow figure for netdev #5437

merged 2 commits into from
Jun 16, 2016

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 13, 2016

I made this graphic for my thesis and I (and @OlegHahm) think it might be nice to have this in the doc.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: doc Area: Documentation Area: drivers Area: Device drivers labels May 13, 2016
@miri64 miri64 added this to the Release 2016.07 milestone May 13, 2016
A-Paul
A-Paul reviewed May 27, 2016
View reviewed changes
drivers/include/net/netdev2.h
* event==NETDEV_EVENT_RX_COMPLETE
* 5. netdev2->event_callback() uses netdev2->driver.recv() to fetch packet
* 1. packet arrives for device
* 2. driver calls @ref netdev2_t::event_callback "netdev->event_callback()"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I your sequential diagram the ISR is calling netdev::event_cb()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the function is called the way it is called here. There is no room in the sequence diagram for the full name and people should IMHO know, that a model is a model and that names are only "Schall und Rauch" ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I don't understand your answer or if my comment was misleading.
I wanted to point out that there are differences between the text and the diagram. You're writing:

  1. driver calls netdev->event_callback() ....

But in the diagram the ISR is calling the function (As far as I can see).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes this is indeed missleading (it's the ISR registered by the driver, but never the less you are right).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh but reading the list item again in total it is mostly clear IMHO

driver calls netdev->event_callback() with event := NETDEV2_EVENT_ISR _(from Interrupt Service Routine)_ and wakes up event handler

Any idea how to make it more clear?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would refer to what I remember about basic principles of documentation.

  • Try to avoid information that is not essential for understanding the specific topic.
  • Use a figure only to provide additional structural overview. Don't repeat information which is already in your text. Instead try to use references to the explaining passages.
    Have you considered just using the sequence numbers as message labels in the SD? That is space saving too.
  • Always remember that the reader is someone who needs your guide through the topic. And not someone who is capable to resolve inconsistencies on its own.
    You claimed the different notations to be all correct. Indeed it is true.
    But if I see different notaions, slightly different function names, etc. Then I try to figure out if the is some meaning in those different representations or I check if I'm the wrong part of the diagram. For that moment I'm completely distracted from the sequence of events you want to show me.

Phew, that turned into something like the "Wort zum Sonntag" :D
Not intended to offend you. I just want to make clear why I'm stressing those stupid questions so much.

driver calls netdev->event_callback() with event := NETDEV2_EVENT_ISR _(from Interrupt Service Routine)_ and wakes up event handler

Any idea how to make it more clear?

Just try what you answered to my questions to get me on the track.

Before the start of the sequence, mention that the ISR has to be registered by the driver as a precondition.
And then e.g.:

  1. ISR calls netdev->event_callback(NETDEV2_EVENT_ISR) and wakes up event handler

Given that I understood what is going on...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never understood as offence, just frustrated with my own incompetence ;-). Will try to take your advice to heart and fix this... tomorrow ;-)

@miri64
Copy link
Member Author

miri64 commented Jun 6, 2016

@A-Paul ping?

@miri64 miri64 assigned A-Paul and unassigned kaspar030 Jun 6, 2016
@A-Paul
Copy link
Member

A-Paul commented Jun 9, 2016

@miri64, slighly OT. Which tool do you use to create sequence diagrams? Are you aware of sdedit?

@miri64
Copy link
Member Author

miri64 commented Jun 9, 2016

@miri64, slighly OT. Which tool do you use to create sequence diagrams? Are you aware of sdedit?

Don't laugh: hand-crafted SVG in vim ;-)

@miri64
Copy link
Member Author

miri64 commented Jun 9, 2016

(how else did you think the XML came out that neat :P)

@miri64
Copy link
Member Author

miri64 commented Jun 15, 2016

Addressed comments.

@A-Paul
Copy link
Member

A-Paul commented Jun 15, 2016 •
edited
Loading

Don't laugh: hand-crafted SVG in vim ;-)

Regarding the font definition block this is outstanding. :D

@miri64
Copy link
Member Author

miri64 commented Jun 15, 2016

Regarding the font definition block this is outstanding. :D

Well I used inkscape as a starting point for formatting (and if I remember correctly a UML editor to get a structural template), but I found that generated XML is much harder to version control than hand-edited, so I thought why not make it proper so that most changes are easier to review.

@A-Paul
Copy link
Member

A-Paul commented Jun 15, 2016

Addressed comments.

Very close to what I imagined. :) Thanks.
ACK.

@A-Paul A-Paul added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 15, 2016
@miri64 miri64 force-pushed the doc/enh/netdev-rx-fig branch from ce3562c to af2e169 Compare June 15, 2016 21:26
@miri64
Copy link
Member Author

miri64 commented Jun 15, 2016

Squashed.

@A-Paul
Copy link
Member

A-Paul commented Jun 16, 2016

Squashed.

Sure? :)

@miri64
Copy link
Member Author

miri64 commented Jun 16, 2016

Yes. One commit for the introduction of the graphic, one for the netdev doc change.

@A-Paul
Copy link
Member

A-Paul commented Jun 16, 2016

Somehow I thought that, in the end, there should be only one commit in a PR.

@A-Paul A-Paul merged commit 87cb266 into RIOT-OS:master Jun 16, 2016
@miri64 miri64 deleted the doc/enh/netdev-rx-fig branch June 16, 2016 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@A-Paul A-Paul

Labels
Area: doc Area: Documentation Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @A-Paul @kaspar030