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

psmoveapi for Windows 8.1 x64 #118

Closed
wants to merge 10 commits into
base: master
from

Conversation

5 participants
@betonme

betonme commented Jul 21, 2014

I've started working on a port for Windows 8.

Now it could be compiled.
The calibration is working.
Added basic support for the navigation controller.

Tested with Win8.1 x64
mingw32-i686-4.8.3-release-win32-sjlj-rt_v3-rev0
cmake-2.8.12.2-win32-x86

betonme added some commits Jul 9, 2014

Removed static declaration of clock_gettime because of linker error
Tested with Win8.1 x64
mingw32-i686-4.8.3-release-win32-sjlj-rt_v3-rev0
cmake-2.8.12.2-win32-x86
Fixed calibration for windows 8
Separated calibration device from input device
Show outdated Hide outdated include/psmove.h
@@ -105,7 +105,7 @@ enum PSMove_Button {
Btn_SHARPSHOOTER_TRIGGER = 1 << 27, /*!< Trigger on Sharp Shooter */
Btn_SHARPSHOOTER_RELOAD = 1 << 28, /*!< Reload on Sharp Shooter */
#if 0
#if 1

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

If you are uncommenting this, please just remove the #if 0 and corresponding #endif instead.

@thp

thp Jul 23, 2014

Owner

If you are uncommenting this, please just remove the #if 0 and corresponding #endif instead.

Show outdated Hide outdated src/psmove.c
/* Navigation Controller */
PSMove_Req_HostBTAddr = 0xF5,
PSMove_Req_GetFirmwareInfo = 0xF9,

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Probably add a comment here /* Motion Controller */ so that users know that this is for the motion controller again, or just move the navigation controller part to the bottom (there's no need for fields in enums to be ordered by value).

@thp

thp Jul 23, 2014

Owner

Probably add a comment here /* Motion Controller */ so that users know that this is for the motion controller again, or just move the navigation controller part to the bottom (there's no need for fields in enums to be ordered by value).

Show outdated Hide outdated src/psmove.c
@@ -251,6 +259,7 @@ struct _PSMove {
/* The handle to the HIDAPI device */
hid_device *handle;
hid_device *handle_calib;

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Indentation

@thp

thp Jul 23, 2014

Owner

Indentation

Show outdated Hide outdated src/psmove.c
@@ -268,6 +277,7 @@ struct _PSMove {
/* Device path of the controller */
char *device_path;
char *device_path_calib;

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Indentation

@thp

thp Jul 23, 2014

Owner

Indentation

Show outdated Hide outdated src/psmove.c
@@ -337,6 +350,14 @@ int _psmove_linux_bt_dev_info(int s, int dev_id, long arg)
#endif /* defined(__linux) */
void

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Should be static void to not expose this function in the shared library and across modules.

@thp

thp Jul 23, 2014

Owner

Should be static void to not expose this function in the shared library and across modules.

Show outdated Hide outdated src/psmove.c
cur_dev = devs;
while (cur_dev) {
while (cur_dev) {

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Indentation

@thp

thp Jul 23, 2014

Owner

Indentation

Show outdated Hide outdated src/psmove.c
#ifdef _WIN32
char *path_calib = (char*) calloc(strlen(path)+1, sizeof(char));

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Also here, indentation

@thp

thp Jul 23, 2014

Owner

Also here, indentation

Show outdated Hide outdated src/psmove.c
/* Set controller type to Navigation Controller */
move->is_navigation = 1;
//move->calibration = psmove_calibration_new(move);

This comment has been minimized.

@thp

thp Jul 23, 2014

Owner

Remove these comments (or better: set move->calibration = NULL explicitly) and same for move->orientation below.

@thp

thp Jul 23, 2014

Owner

Remove these comments (or better: set move->calibration = NULL explicitly) and same for move->orientation below.

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Jul 23, 2014

Owner

Basic review done. Please check the comments. Also, please stick to the code style guidelines (keep indentation and spacing / braces aligned with the rest of the surrounding code).

Owner

thp commented Jul 23, 2014

Basic review done. Please check the comments. Also, please stick to the code style guidelines (keep indentation and spacing / braces aligned with the rest of the surrounding code).

@betonme

This comment has been minimized.

Show comment
Hide comment
@betonme

betonme Jul 31, 2014

Hi, thanks for Your review. I've added the requested changes.
Frank

betonme commented Jul 31, 2014

Hi, thanks for Your review. I've added the requested changes.
Frank

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Sep 26, 2014

Owner

Sorry for taking so long to respond. Can you fix the indentation? There are still some issues with the coding style, please keep the indentation the same as the surrounding code.

Also, maybe @nitsch and @whitingjp can take a look at this patch to see if it works for them?

Owner

thp commented Sep 26, 2014

Sorry for taking so long to respond. Can you fix the indentation? There are still some issues with the coding style, please keep the indentation the same as the surrounding code.

Also, maybe @nitsch and @whitingjp can take a look at this patch to see if it works for them?

@nitsch

This comment has been minimized.

Show comment
Hide comment
@nitsch

nitsch Sep 26, 2014

Does this really solve the issue for you? Like I stated elsewhere, the whole definition seems unnecessary. Testing your fix on my setup (still unchanged, but verified on Windows 7 by now) still produces warnings about redefining CLOCK_MONOTONIC. Please check if removing this complete section also works for you, i.e. removing the WIN32 check from

#if defined(__APPLE__) || defined(_WIN32)

Also, if we end up using you fix, we should make sure it does not break OS X builds. Can anybody test this?

nitsch commented on 3823590 Sep 26, 2014

Does this really solve the issue for you? Like I stated elsewhere, the whole definition seems unnecessary. Testing your fix on my setup (still unchanged, but verified on Windows 7 by now) still produces warnings about redefining CLOCK_MONOTONIC. Please check if removing this complete section also works for you, i.e. removing the WIN32 check from

#if defined(__APPLE__) || defined(_WIN32)

Also, if we end up using you fix, we should make sure it does not break OS X builds. Can anybody test this?

This comment has been minimized.

Show comment
Hide comment
@betonme

betonme Sep 29, 2014

Owner

You're right removing the static does produce warnings, but the make process will finish successful and it works. With the static declaration, I can't build it.

Owner

betonme replied Sep 29, 2014

You're right removing the static does produce warnings, but the make process will finish successful and it works. With the static declaration, I can't build it.

This comment has been minimized.

Show comment
Hide comment
@nitsch

nitsch Sep 29, 2014

I got that last part. But what about removing the whole section altogether? No CLOCK_MONOTONIC and no clock_gettime(). MinGW should have its own.

nitsch replied Sep 29, 2014

I got that last part. But what about removing the whole section altogether? No CLOCK_MONOTONIC and no clock_gettime(). MinGW should have its own.

This comment has been minimized.

Show comment
Hide comment
@betonme

betonme Sep 30, 2014

Owner

Ok, it works, if I use:

Previously I've used mingw32-i686-4.8.3-release-win32-sjlj-rt_v3-rev0.

Owner

betonme replied Sep 30, 2014

Ok, it works, if I use:

Previously I've used mingw32-i686-4.8.3-release-win32-sjlj-rt_v3-rev0.

@nitsch

This comment has been minimized.

Show comment
Hide comment
@nitsch

nitsch Sep 27, 2014

Contributor

I suggest you move the Navigation-related commit to a separate feature branch. It is not specific to Windows and it also makes it easier to test and improve.

Contributor

nitsch commented Sep 27, 2014

I suggest you move the Navigation-related commit to a separate feature branch. It is not specific to Windows and it also makes it easier to test and improve.

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Oct 30, 2014

Owner

@nitsch what do you think of the current pull request?

Owner

thp commented Oct 30, 2014

@nitsch what do you think of the current pull request?

@nitsch

This comment has been minimized.

Show comment
Hide comment
@nitsch

nitsch Oct 30, 2014

Contributor

As far as I can tell the thing that is left after the recent changes and revert only targets the calibration. But I have no idea what problem it is trying to solve (and what exactly makes this workaround work compared to the current implementation).

I could not find any problems with accessing the calibration data on Windows. I cannot test on Windows 8 though, so it might be something specific to this version. Some clarification (and ideally some documentation) would be nice.

Contributor

nitsch commented Oct 30, 2014

As far as I can tell the thing that is left after the recent changes and revert only targets the calibration. But I have no idea what problem it is trying to solve (and what exactly makes this workaround work compared to the current implementation).

I could not find any problems with accessing the calibration data on Windows. I cannot test on Windows 8 though, so it might be something specific to this version. Some clarification (and ideally some documentation) would be nice.

@betonme

This comment has been minimized.

Show comment
Hide comment
@betonme

betonme Nov 5, 2014

On Windows XP and 7, I have also no problems accessing the calibration data. With Windows 8 I can access it only via the

On all Windows Versions I can access the calibration data onlvy with the existing ugly workaround:
Convert "col02" path to "col01" path.

But with the changed handle the function call
hid_get_feature_report(move->handle, btg, sizeof(btg));
fails on Windows 8. If the handle is unchanged it is working as expected.

betonme commented Nov 5, 2014

On Windows XP and 7, I have also no problems accessing the calibration data. With Windows 8 I can access it only via the

On all Windows Versions I can access the calibration data onlvy with the existing ugly workaround:
Convert "col02" path to "col01" path.

But with the changed handle the function call
hid_get_feature_report(move->handle, btg, sizeof(btg));
fails on Windows 8. If the handle is unchanged it is working as expected.

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Feb 15, 2015

Owner

How does this relate to #149 ? Should we aim to get both pull requests merged into a single pull request? This pull request looks cleaner, the other has indentation issues, but both try to solve the same problem? // cc @cboulay

Owner

thp commented Feb 15, 2015

How does this relate to #149 ? Should we aim to get both pull requests merged into a single pull request? This pull request looks cleaner, the other has indentation issues, but both try to solve the same problem? // cc @cboulay

@cboulay

This comment has been minimized.

Show comment
Hide comment
@cboulay

cboulay Feb 15, 2015

Contributor

@thp They do both try to solve the same problem and #149 uses the basic same technique that this one did. However, this one never worked for me. There is a small difference that I can't recall right now and I don't have time to reinvestigate because I have to take my daughter to swimming. By the time I get back to working on this, Europe will be asleep.

Contributor

cboulay commented Feb 15, 2015

@thp They do both try to solve the same problem and #149 uses the basic same technique that this one did. However, this one never worked for me. There is a small difference that I can't recall right now and I don't have time to reinvestigate because I have to take my daughter to swimming. By the time I get back to working on this, Europe will be asleep.

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Mar 1, 2015

Owner

So this is superseded by #149 then, and #149 should be reviewed/merged instead of this one?

Owner

thp commented Mar 1, 2015

So this is superseded by #149 then, and #149 should be reviewed/merged instead of this one?

@cboulay

This comment has been minimized.

Show comment
Hide comment
@cboulay

cboulay Mar 1, 2015

Contributor

Yes, I believe so. But, as @betonme mentioned in #149, the changes in #149 (and I suspect this PR, too) expose another problem. I have a question for you about that, but I'll ask it in #149.

Contributor

cboulay commented Mar 1, 2015

Yes, I believe so. But, as @betonme mentioned in #149, the changes in #149 (and I suspect this PR, too) expose another problem. I have a question for you about that, but I'll ask it in #149.

@Lirrec

This comment has been minimized.

Show comment
Hide comment
@Lirrec

Lirrec Sep 24, 2015

Contributor

As #149 is already merged, shouldn't this one be closed?

Contributor

Lirrec commented Sep 24, 2015

As #149 is already merged, shouldn't this one be closed?

@thp

This comment has been minimized.

Show comment
Hide comment
@thp

thp Sep 27, 2015

Owner

Closing this now. Please rebase/reopen if it's still relevant.

Owner

thp commented Sep 27, 2015

Closing this now. Please rebase/reopen if it's still relevant.

@thp thp closed this Sep 27, 2015

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