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

Fix rumble + ifdefs + rumble documentation #594

Open
wants to merge 17 commits into
base: develop/2.4.0
Choose a base branch
from

Conversation

Arceveti
Copy link
Collaborator

@Arceveti Arceveti commented Mar 9, 2023

Fixes rumble and rumble ifdefs, and documents rumble functionality.

@Arceveti Arceveti requested a review from gheskett as a code owner March 9, 2023 17:04
@Arceveti Arceveti requested review from thecozies, FazanaJ and Reonu March 9, 2023 17:15
@Arceveti Arceveti added this to the 2.1 milestone Mar 9, 2023
@Arceveti Arceveti added the high priority Important, non-critical issue or feature / high priority label Mar 9, 2023
@Arceveti Arceveti changed the title Fix rumble + ifdefs Fix rumble + ifdefs + rumble documentation Mar 9, 2023
@@ -14,7 +14,7 @@
* Enables Rumble Pak Support.
* Currently not recommended, as it may cause random crashes.
*/
// #define ENABLE_RUMBLE (1 || VERSION_SH)
#define ENABLE_RUMBLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either this should be disabled by default, or if it is safe to always leave on, the comment two lines up should be modified/deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the comment. I think it should be on by default since it's never crashed in any of my testing with these changes. I think that comment was about the old PJ64 crash from around 2 years ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not totally sure about it being left on even if it doesnt cause too much instability just because its extra code being added and most people won't care about it, and we can't be totally certain its not unstable

include/config/config_safeguards.h Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@

/*---------------------------------------------------------------------*
Copyright (C) 1998 Nintendo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally think we shouldn't be touching any of the include/n64/PR/ files with any formatting changes whatsoever, unless they revolve around code actually written by us.

@Arceveti Arceveti requested a review from gheskett August 11, 2023 20:01
include/ique/PR/os_pfs.h Outdated Show resolved Hide resolved
include/ique/PR/os_pfs.h Outdated Show resolved Hide resolved
#define CHNL_ERR_NORESP (0b1 << 7) /* 0x80: Bit 7 (Rx): No response error */
#define CHNL_ERR_OVERRUN (0b1 << 6) /* 0x40: Bit 6 (Rx): Overrun error */
#define CHNL_ERR_FRAME (0b1 << 7) /* 0x80: Bit 7 (Tx): Frame error */
#define CHNL_ERR_COLLISION (0b1 << 6) /* 0x40: Bit 6 (Tx): Collision error */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use 1 instead of 0b1 for these (last one's probably fine though)

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed

#define CHNL_ERR_NORESP (0b1 << 7) /* 0x80: Bit 7 (Rx): No response error */
#define CHNL_ERR_OVERRUN (0b1 << 6) /* 0x40: Bit 6 (Rx): Overrun error */
#define CHNL_ERR_FRAME (0b1 << 7) /* 0x80: Bit 7 (Tx): Frame error */
#define CHNL_ERR_COLLISION (0b1 << 6) /* 0x40: Bit 6 (Tx): Collision error */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

s32 sRumblePakActive = FALSE;
s32 sRumblePakErrorCount = 0;
s32 gRumblePakTimer = 0;
_Bool sRumblePakThreadActive = FALSE; // Set to TRUE when the rumble thread starts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

_Bool doesn't exist anywhere else in the repo. Not sure if it comes with any downsides, but I might just stick with u8 for consistency sake. (At minimum, make sure it compiles with GCC 9, or it's definitely a no-go.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed, this should be a u8

@gheskett gheskett added the bug Something isn't working label Sep 22, 2023
@gheskett
Copy link
Collaborator

Moving to 2.2/3.0.

@gheskett gheskett removed this from the 2.1 milestone Sep 22, 2023
@gheskett gheskett modified the milestones: 3.0, 2.2 Sep 27, 2023
@gheskett gheskett changed the base branch from develop/2.1.0 to develop/2.2.0 September 27, 2023 01:47
@Arceveti
Copy link
Collaborator Author

Arceveti commented Oct 6, 2023

Moving to new PR

@Arceveti Arceveti closed this Oct 6, 2023
@Arceveti
Copy link
Collaborator Author

Arceveti commented Oct 6, 2023

Wrong PR lol

@Arceveti Arceveti reopened this Oct 6, 2023
@@ -14,7 +14,7 @@
* Enables Rumble Pak Support.
* Currently not recommended, as it may cause random crashes.
*/
// #define ENABLE_RUMBLE (1 || VERSION_SH)
#define ENABLE_RUMBLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

not totally sure about it being left on even if it doesnt cause too much instability just because its extra code being added and most people won't care about it, and we can't be totally certain its not unstable

#define CHNL_ERR_NORESP (0b1 << 7) /* 0x80: Bit 7 (Rx): No response error */
#define CHNL_ERR_OVERRUN (0b1 << 6) /* 0x40: Bit 6 (Rx): Overrun error */
#define CHNL_ERR_FRAME (0b1 << 7) /* 0x80: Bit 7 (Tx): Frame error */
#define CHNL_ERR_COLLISION (0b1 << 6) /* 0x40: Bit 6 (Tx): Collision error */
Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed

s32 sRumblePakActive = FALSE;
s32 sRumblePakErrorCount = 0;
s32 gRumblePakTimer = 0;
_Bool sRumblePakThreadActive = FALSE; // Set to TRUE when the rumble thread starts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed, this should be a u8

@gheskett gheskett modified the milestones: 2.2, 2.3 Mar 2, 2024
@gheskett gheskett changed the base branch from develop/2.2.0 to develop/2.3.0 March 2, 2024 13:48
@gheskett gheskett modified the milestones: 2.3, 2.4 Jul 4, 2024
@gheskett gheskett changed the base branch from develop/2.3.0 to develop/2.4.0 July 4, 2024 07:12
@riggles1
Copy link

riggles1 commented Oct 2, 2024

Hi! Is this related to non-functional inputs if the rumble pak is present? (start with it plugged in, or plugged in during play, the controller stops working after that).

In my case I built a basically vanilla SM64 with minor bugfixes (like invisible walls) and performance improvements, but with Rumble Pak support. Works perfectly, except for the rumble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important, non-critical issue or feature / high priority
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants