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

Adding code that checks if the nav sd card is installed, and if it is… #123

Closed
wants to merge 3 commits into from
Closed

Adding code that checks if the nav sd card is installed, and if it is… #123

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21fa1dd
Adding code that checks if the nav sd card is installed, and if it is…
aidandj Mar 1, 2018
3a4007c
I should do real testing before commiting things. This one works
aidandj Mar 1, 2018
1b904b0
Renaming variable because it wasn't very descriptive
aidandj Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion mazda/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <dbus-c++/dbus.h>
#include <dbus-c++/glib-integration.h>
#include <sys/time.h>
#include <sys/stat.h>

#include "hu_uti.h"
#include "hu_aap.h"
Expand All @@ -34,6 +35,8 @@

#define HMI_BUS_ADDRESS "unix:path=/tmp/dbus_hmi_socket"
#define SERVICE_BUS_ADDRESS "unix:path=/tmp/dbus_service_socket"
// Check the content folder. sd_nav still exists without the card installed
#define SD_CARD_PATH "/tmp/mnt/sd_nav/content"

__asm__(".symver realpath1,realpath1@GLIBC_2.11.1");

Expand Down Expand Up @@ -114,7 +117,27 @@ static void gps_thread_func(std::condition_variable& quitcv, std::mutex& quitmut
location->set_latitude(static_cast<int32_t>(data.latitude * 1E7));
location->set_longitude(static_cast<int32_t>(data.longitude * 1E7));

location->set_bearing(static_cast<int32_t>(data.heading * 1E6));
// If the sd card exists then reverse heading. This should only be used on installs that have the
// reversed heading issue. Maybe gate it by a config file
const char* sdCardFolder;
sdCardFolder = SD_CARD_PATH;
struct stat sb;
double newHeading;

if (stat(sdCardFolder, &sb) == 0 && S_ISDIR(sb.st_mode))
Copy link

@mishan mishan Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Although stat() is super cheap, how often does this method get called? Can we perhaps cache the existence / non-existence of the directory and just check a variable here instead? Too many syscalls can be performance impacting and these CMUs are already mighty weak.

Copy link
Author

Choose a reason for hiding this comment

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

Every second. The problem with caching it is if you pull it out while running it will reverse. This code allows you to plug and unplug the sdcard while running google maps

Copy link

Choose a reason for hiding this comment

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

I understand this -- is that really an important use case to cover, though? My understanding is that people will either have an SD card or not.

I think you're on the right track here, but I'd rather not add another syscall to perform every second. Can we check periodically and flip a flag if needed? A check every 30 seconds? People pulling out and putting their SD card back in seems very edge case-y. I know it's great for testing though :)

Copy link

Choose a reason for hiding this comment

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

I mean, we could always just tell users they have to restart AA if they're going to pull out or re-insert their Mazda Nav SD card -- that doesn't sound unreasonable to me. And it makes the engineering solution simpler. My personal philosophy is that sometimes it's better to set clear product requirements than to engineer for edge cases, especially if it costs any performance

Copy link
Author

Choose a reason for hiding this comment

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

That's definitely a better idea. I'd have to do some more investigating to do a timer.

Copy link
Collaborator

@Trevelopment Trevelopment Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yea I did a test and the performance impact from that check is definitely noticeable and funny thing is it didn't even flip my heading the wrong way even when I took out the check.

Copy link
Author

Choose a reason for hiding this comment

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

You have to wait for a bit for it to flip. But interesting.

I will install a version without the check soon. Did you push your changes to any branch?

Copy link
Collaborator

@Trevelopment Trevelopment Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

no but I only used lines 125, 129-133 & 140 thats why its so funny because its definitely +-180 off but it somehow still knows how to correct itself.

Copy link
Collaborator

@Trevelopment Trevelopment Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

After 2 days still works perfectly for me without flipping the heading or any other unexpected issues with or without a Nav SD card connected. Tested it on some other people too and nobody reported a reversed heading whether they had the issue to begin with or not. Its very strange that it wouldn't flip the heading for those that didn't have the issue to begin with but so far thats what its looking like. I would say if you push a few changes like just poll for the Nav SD card once at the start that would be sufficient and if after some more testing it is confirmed that it doesn't cause an issue for anyone then I would say merge it because it definitely fixes the issue for those who have it, that has pretty much been confirmed by several people.

Copy link

@Peck07 Peck07 Aug 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I've merged this code with my config file combination:
Peck07@b26e717

{
newHeading = data.heading + 180;
if (newHeading >= 360)
{
newHeading = newHeading - 360;
}
}
else
{
newHeading = data.heading;
}

location->set_bearing(static_cast<int32_t>(newHeading * 1E6));
//assuming these are the same units as the Android Location API (the rest are)
double velocityMetersPerSecond = data.velocity * 0.277778; //convert km/h to m/s
location->set_speed(static_cast<int32_t>(velocityMetersPerSecond * 1E3));
Expand Down