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

AP_Notify: Support for OLED display by Alexey Kozin #5135

Closed

Conversation

@dipspb
Copy link
Contributor

dipspb commented Nov 4, 2016

#5082 (review)

It seems github refused to accept rebased commits that were included in #5082
So I have to create new branch and submit this new PR to provide all commits squashed to a single one with "git rebase -i HEAD~18".

@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

The github app is junk, I recommend sourcetree. What you need to do is force push after the rebase -i

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

@magicrub Thank you, Tom. I didn't mean github app. I mean github site. When I pushed rebased commits to the same branch it doesn't complain anything. But it seems nothing has been changed on side of github repo. It still have all old commits as not rebased. Then I attempted to repeat push with force option and still have no luck. Then I created new branch.

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

Minor fix added as separate commit e289523 into this PR because we have just discovered issue in hardware test and fixed it.

@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

@dipspb your changes were in effect on the other PR

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

@magicrub Not quite sure I've got your point. What do you mean?

@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

@dipspb I mean the updates to the branch are on that PR. Everything was fine. Changing to a new PR was unnecessary. However, since you're making a new branch and a new PR it would have been nice to rebase onto master while you were at it but that's OK, there's no merge conflicts so it happens auto-magically

@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

I hate to be the one bringing the bad news, but you've squashed your commits too much. In a PR like this we'd really like to see the commits done to the vehicle be in a separate commit.

@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

If you aren't able to do that then maybe the one merging it can

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

@magicrub I remastered my commits in accordance with recommendations from @mirkix #5082 (comment) and following "submitting patches" document http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html

And I have not a minor advice on what commits should be left separate. Nobody warned me and so on..

So my three questions at this moment:

  1. How would I guess it?
  2. Where I can read plain and clear document the covers issue you have just mentioned?
  3. What I need to do?

Thank you very much in advance

p.s. The person who is merging, it's me.

@OXINARF
Copy link
Member

OXINARF commented Nov 4, 2016

@dipspb It is written there:

"Commits should be small, and do just one thing. If a change touches multiple libraries then there should be a separate commit per library, and a separate commit per vehicle directory. This is true even if it means that intermediate commits break the build."

Along with:

Commit messages should be of the form:

Subsystem: brief description

Longer description...

Anyway, there are previous comments I made in the previous PR that still haven't been addressed. Also, it has been found by Tridge that this driver (along with others) are doing I2C transactions in the main thread and although that looks to be fine in Linux it is a no-no for Pixhawk. That needs to be fixed before this can go in.

P.S.: You are not merging, you are proposing a change in the form of a pull request. Someone from the team will merge this to master.

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

@OXINARF thank you. Will redo. So one more branch/PR?

this driver (along with others) are doing I2C transactions in the main thread and although that looks to be fine in Linux it is a no-no for Pixhawk. That needs to be fixed before this can go in.

How and where it need to be fixed? What about other drivers?

P.S. Sure, you are right. Here is about midnight and I falling asleep a bit. I mean 'I'm a person who is remastering these PRs'

@dipspb
Copy link
Contributor Author

dipspb commented Nov 4, 2016

Ok. I can see 4 (sort of) unresolved issues to do:

  1. One missed usage of sprintf. To be done.
  2. "ARMED message only when armed" #5082 (comment)
  3. I2C issue(s) #5082 (comment) + threading issue
  4. Separate squashed commits for: AP_Notify, ArduCopter and ArduPlane

Right? I can see what to do with 1 and 4. And I need to see what to do with 2 and 3.

@OXINARF
Copy link
Member

OXINARF commented Nov 4, 2016

@dipspb Several points:

  • when I said there are comments from me that weren't addressed I was referring to the first two points of #5082 (comment)
  • regarding the I2C fixes you only need to worry about this driver since we can add it to Pixhawk without fixing it. You can see at 91cc8b7 how Tridge fixed it. We also need to understand why @kozinalexey was seeing artifacts with the big I2C transfer (your point number 3).
  • regarding the ARMED I just wanted to understand if that was the desired functionality, I haven't answered as I think @kozinalexey gave a good answer
  • you don't need to do another PR, you need to split the commit and force push. To do that you will either need to use the git command line or use a GUI to work with Git (Tom recommended SourceTree and I can recommend it too). If you need help with this we can help in Gitter.
@magicrub
Copy link
Contributor

magicrub commented Nov 4, 2016

p.s. The person who is merging, it's me.

I meant the person who's merging into Ardupilot. We can do it

private:
static NotifyDevice* _devices[];

static float _voltage;
static uint8_t _control_mode;

This comment has been minimized.

Copy link
@magicrub

magicrub Nov 7, 2016

Contributor

why are _voltage and _control_mode static?

This comment has been minimized.

Copy link
@dipspb

dipspb Nov 21, 2016

Author Contributor

fixed with new set of commits in the same branch

@dipspb dipspb force-pushed the dipspb:feature-OLED-by-Alexey_Kozin-rework branch from 5dcb104 to d092f32 Nov 21, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 21, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 21, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 21, 2016
@dipspb
Copy link
Contributor Author

dipspb commented Nov 21, 2016

Commits were reworked.

@dipspb dipspb force-pushed the dipspb:feature-OLED-by-Alexey_Kozin-rework branch from d092f32 to 81180ca Nov 22, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 22, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 22, 2016
dipspb added a commit to dipspb/ardupilot that referenced this pull request Nov 22, 2016
@dipspb
Copy link
Contributor Author

dipspb commented Nov 22, 2016

Extra accessor methods removed. Extra fields like _voltage and _control_mode were moved to AP_Notify::flags.

@rmackay9
Copy link
Contributor

rmackay9 commented Nov 26, 2016

I've had a quick look at this, the duplication of the flight mode names doesn't make me too happy. It's something we would need to remember to update each time we add a new flight mode. Not the end of the world but not great. We already have a print_flight_mode function in each vehicle class although it sends to a "BetterStream" and it would probably be ugly to pass this into Notify.

I was actually surprised that this was written as a Notify object. I was expecting it to be more like the FrSky protocol or a telemetry link to the ground station. It would be nice for example to have it display the messages that normally appear on the mission planner HUD.. like the pre-arm check failures.

Update: maybe I need to read this PR more, I see from a picture posted in gitter than the pre-arm messages do seem to be printed (or at least "Compass not calib" is being printed.

@dipspb
Copy link
Contributor Author

dipspb commented Nov 26, 2016

@rmackay9 First line of OLED display shows not just cropped message. It runs like a ticker showing complete HUD messages. Here is how it's running:
https://www.youtube.com/watch?v=VON2CzIsfOI

@dipspb
Copy link
Contributor Author

dipspb commented Nov 26, 2016

Hello, @rmackay9 Unsure I've got your point about Notification vs Telemetry. Could you pls to clarify in more details? What's wrong with Notify object? We didn't do that, it was already implemented this way by @mirkix .

That's for mode names, IMHO it shouldn't be a big deal to refactor print_flight_mode by extracting get_flight_mode_string from it. Any other ideas are appreciated.

The only thing that frustrating me really much is a fact that review feedback notes are arriving one-by-one with a few days gap. May I have review notes for this PR all at once so I would able to resolve all of them and then to be sure PR will be accepted?

@dipspb dipspb force-pushed the dipspb:feature-OLED-by-Alexey_Kozin-rework branch from 6cb1dc0 to 2d77208 Jan 1, 2017
@dipspb
Copy link
Contributor Author

dipspb commented Jan 1, 2017

snprintf(..., "%s", ...) changed to strncpy

@magicrub
Copy link
Contributor

magicrub commented Jan 1, 2017

Thanks @dipspb . I just did a check for snprintf and vsnprintf and it's used almost 70 times where strncpy is only used about 25 times. Maybe I gave you wrong advice?

Can anyone else chime in on this to sanity check me?

@dipspb
Copy link
Contributor Author

dipspb commented Jan 1, 2017

@magicrub I pretty sure it was really good advice for that particular method. Thank you!

@dipspb
Copy link
Contributor Author

dipspb commented Jan 3, 2017

Hi @rmackay9 , do you have any success testing your OLED modules with my final commits?

dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 7, 2017
dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 7, 2017
dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 7, 2017
dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 8, 2017
dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 8, 2017
dipspb added a commit to dipspb/ardupilot that referenced this pull request Jan 8, 2017
@rmackay9 rmackay9 added this to the AC 3.5.0 milestone Jan 9, 2017
rmackay9 added a commit to rmackay9/rmackay9-ardupilot that referenced this pull request Jan 17, 2017
@rmackay9
Copy link
Contributor

rmackay9 commented Jan 17, 2017

I've received a display from @dipspb and it works! I don't understand why my existing displays from adafruit don't work. Is the aliexpress link from above the best source of displays?

I've rebased on master and resolved a few conflicts. https://github.com/rmackay9/rmackay9-ardupilot/commits/oled-display2.

There are a few potential issues that I hope to make more progress on tomorrow:

  • the front-end/back-end split is a slightly different method than we currently use (see other libraries like AP_Buzzer to see how we normally do it) so I will like re-format it to look more like other libraries.
  • @tridge is concerned about the I2C traffic especially when flying so I will test this.
  • @OXINARF (and perhaps others) will be unhappy about the vehicle specific #defines in the library to provide the flight modes.
@kozinalexey
Copy link
Contributor

kozinalexey commented Jan 17, 2017

@rmackay9 oled display have i2c address solder jumper. maybe adafruit model have wrong default setting

@magicrub
Copy link
Contributor

magicrub commented Jan 17, 2017

To address the excess i2c traffic during flight, can we disable this feature while flying and/or armed to get this PR in and then fix that later?

@kozinalexey
Copy link
Contributor

kozinalexey commented Jan 17, 2017

display do not have traffic in armed state, after ARMING it clear screen , show message "ARMED" and stop any updates

@OXINARF
Copy link
Member

OXINARF commented Jan 17, 2017

@OXINARF (and perhaps others) will be unhappy about the vehicle specific #defines in the library to provide the flight modes.

I couldn't think of a better option, so I'm not against it. It's not great to have it, but until someone does an AP_Telemetry library (which I envision as having subclasses in every vehicle) I think we'll continue to have these defines.

@rmackay9
Copy link
Contributor

rmackay9 commented Jan 18, 2017

@OXINARF, Thanks. As a side note, I've also been thinking that we need an AP_Telemetry library which would absorb our GCS_MAVLink and FRSky telemetry libraries.

@lucasdemarchi
Copy link
Contributor

lucasdemarchi commented Jan 19, 2017

I couldn't think of a better option, so I'm not against it. It's not great to have it, but until someone does an AP_Telemetry library (which I envision as having subclasses in every vehicle) I think we'll continue to have these defines.

A method inside each vehicle that returns the flight mode would suffice and avoid the ifdef. It would be very simple while a more elaborate solution is not done.

@OXINARF
Copy link
Member

OXINARF commented Jan 19, 2017

I thought of that but I'm not a fan of vehicles calling libraries calling vehicles. Also, it wouldn't be a direct connection in this case because the Display object is created in AP_Notify (and that is what is created in the vehicle), which makes it even worse for me.

@lucasdemarchi
Copy link
Contributor

lucasdemarchi commented Jan 19, 2017

Still better than a ifdef.

AP_Vehicle::get_flight_mode(), to be implemented by each vehicle. No reference passing, straight plain old boring function :)

@OXINARF
Copy link
Member

OXINARF commented Jan 19, 2017

I consider them almost equally bad that I don't care about the ifdef.

@rmackay9
Copy link
Contributor

rmackay9 commented Jan 19, 2017

I'm going to close this PR because we have an updated PR now: #5581

@rmackay9 rmackay9 closed this Jan 19, 2017
EShamaev added a commit to EShamaev/ardupilot that referenced this pull request Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.