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

MINIFI-218: Support for GPSD integration #52

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jdye64
Contributor

jdye64 commented Feb 21, 2017

No description provided.

@phrocker

Minor nitpicks, I wouldn't necessarily decline because of them.

private:
//! Logger
Logger *_logger;

This comment has been minimized.

@phrocker

phrocker Feb 21, 2017

Contributor

logger_ is the google cpp style guide approach. Same with all of these.

This comment has been minimized.

@jdye64

jdye64 Feb 21, 2017

Contributor

No, I agree with you about this. I'll change them to make them follow the google guidelines. Best to start those new habits now =)

return;
}
for (;;) {

This comment has been minimized.

@phrocker

phrocker Feb 21, 2017

Contributor

Could you wait on a condition in case this processor is forced shutdown.

#include "../FlowFileRecord.h"
TEST_CASE("Test GPSD Create", "[TestGPSEventRecord]"){

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Would we be able to create some processor specific tests?

This comment has been minimized.

@jdye64

jdye64 Feb 23, 2017

Contributor

certainly. I'm wondering the best way to do this at the moment. I want to use gpsfake for this but thinking through the best way to do so. Using gpsfake would require another system dependency since it is an extra GPSD service itself. That would be the most robust way for testing this however

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Might make sense to roll in a mocking framework? https://github.com/eranpeer/FakeIt seems similarly lightweight to catch. @phrocker any recommendations or preferences?

This comment has been minimized.

@jdye64

jdye64 Feb 23, 2017

Contributor

@apiri and @phrocker I'm leaning on you guys for recommendations around the tests here. I was planning on using http://www.catb.org/gpsd/gpsfake.html

This comment has been minimized.

@phrocker

phrocker Mar 8, 2017

Contributor

yeah I initially approved because I wasn't sure there was a good way to test this. I didn't know about gpsfake, and would love if there was a way to roll that in. Woe is me thinking there isn't a good way to test something.

@apiri

This comment has been minimized.

Contributor

apiri commented Feb 23, 2017

Could we also get travis updated to include this in the build by default?

README.md Outdated
@@ -110,6 +118,9 @@ $ brew install cmake \
leveldb \
ossp-uuid \
boost \ openssl
# If building with GPS support
$ brew install gps

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Believe this should be gpsd

This comment has been minimized.

@jdye64

jdye64 Feb 23, 2017

Contributor

Its actually is just gps for OS X. Its the gps.h headers and such that is needed. However it might make sense to also install gpsd so I'll add that

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Hmm... gps doesn't show up as a package for me in brew.

brew search gps
bgpstream         gpsbabel          gpsd ✔            gpsim             osm-gps-map
Caskroom/cask/gpsdump

Am I overlooking something?

#include "ProcessContext.h"
#include "ProcessSession.h"
#include <libgpsmm.h>

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Having issues on OS X 10.11 with the following:

In file included from /Users/apiri/Development/code/nifi-minifi-cpp/libminifi/src/gps/GetGPS.cpp:43:
In file included from /usr/local/include/libgpsmm.h:12:
/usr/local/include/gps.h:1887:8: error: definition of type 'policy_t' conflicts with
      typedef of the same name
struct policy_t {
       ^
/usr/include/mach/policy.h:79:17: note: 'policy_t' declared here
typedef int                             policy_t;
                                        ^
In file included from /Users/apiri/Development/code/nifi-minifi-cpp/libminifi/src/gps/GetGPS.cpp:43:
In file included from /usr/local/include/libgpsmm.h:12:
/usr/local/include/gps.h:2014:12: error: elaborated type refers to a typedef
    struct policy_t policy;     /* our listening policy */
           ^
/usr/include/mach/policy.h:79:17: note: declared here
typedef int                             policy_t;
                                        ^
2 errors generated.
make[2]: *** [libminifi/CMakeFiles/minifi_gps.dir/src/gps/GetGPS.cpp.o] Error 1

README.md Outdated
@@ -91,6 +93,9 @@ $ yum install cmake \
leveldb-devel leveldb \
libuuid libuuid-devel \
boost-devel \ libssl-dev
# If building with GPS support
$ yum install libgps-devel

This comment has been minimized.

@apiri

apiri Feb 23, 2017

Contributor

Believe this should gpsd-devel for yum based distros

@@ -289,6 +289,12 @@ Processor *FlowControllerImpl::createProcessor(std::string name, uuid_t uuid) {
return NULL;
}
#ifdef BUILD_GPS

This comment has been minimized.

@jdye64

jdye64 Feb 23, 2017

Contributor

This was not very smart of me ... this will cause an error to be thrown that the processor of type GetGPS does not exist at runtime because the last else condition will be triggered before this. I need to move this higher up the condition before the last else.

@jdye64 jdye64 changed the title from Support for GPSD integration to MINIFI-218: Support for GPSD integration Mar 2, 2017

@jdye64

This comment has been minimized.

Contributor

jdye64 commented Oct 9, 2017

This poor PR has been neglected. I think that now @phrocker PR for extensions has been committed its a good time to make another pass at this and place it in that extensions format

@phrocker

This comment has been minimized.

Contributor

phrocker commented Nov 7, 2017

@jdye64 I think this PR has been superseded. I've taken your PR in #172 and updated it. This can be closed.

@achristianson

This comment has been minimized.

Contributor

achristianson commented Nov 17, 2017

Should this PR be closed? I believe we got gpsd in with another recent commit.

@achristianson

This comment has been minimized.

Contributor

achristianson commented Nov 17, 2017

f09ad61 is in master

@phrocker

This comment has been minimized.

Contributor

phrocker commented Nov 17, 2017

@achristianson yes per the comment nine days ago. I've spoken to Jeremy offline.

@jdye64

This comment has been minimized.

Contributor

jdye64 commented Dec 13, 2017

Sorry for the late response here folks. I'm going to close this issue because as stated it has already been taken care of elsewhere.

@jdye64 jdye64 closed this Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment