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

FWT-11 #2 hallelujah effect sensor #1

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

ol2764RIT
Copy link

@ol2764RIT ol2764RIT commented Feb 27, 2024

Created the very new hallSensor class

-Goes through a GPIO read and assigns the correct case
-returns time
-returns distance traveled and speed

Created main for WSS using hall effect sensors
-Created representation of WSS using two hall effect sensors for back and front wheels
-Allow can transmission of data for validation of proper functionality

@ol2764RIT ol2764RIT requested a review from a team February 27, 2024 00:39
Copy link
Contributor

@DiegoLHendrix DiegoLHendrix left a comment

Choose a reason for hiding this comment

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

Thank you Oleg very cool

Copy link

@aclowmclaughlin aclowmclaughlin left a comment

Choose a reason for hiding this comment

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

Some minor comment changes but I don't wanna rereview so I trust you'll do them

include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
src/dev/hallSensor.cpp Outdated Show resolved Hide resolved
targets/hallSensor/main.cpp Outdated Show resolved Hide resolved
Copy link

@mjmagee991 mjmagee991 left a comment

Choose a reason for hiding this comment

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

Some more to do

CMakeLists.txt Show resolved Hide resolved
include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
src/dev/hallSensor.cpp Outdated Show resolved Hide resolved
include/dev/hallSensor.hpp Outdated Show resolved Hide resolved
targets/CMakeLists.txt Outdated Show resolved Hide resolved
targets/hallSensor/main.cpp Outdated Show resolved Hide resolved
targets/hallSensor/main.cpp Outdated Show resolved Hide resolved
@ol2764RIT ol2764RIT marked this pull request as draft March 5, 2024 13:19
@ol2764RIT
Copy link
Author

will need to redo as I require a GPIO IRQ Handler

@mjmagee991 mjmagee991 changed the title #2 hallelujah effect sensor FWT-11 #2 hallelujah effect sensor Jul 5, 2024
Copy link

@tmb5932 tmb5932 left a comment

Choose a reason for hiding this comment

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

A couple comments 🐸 🐸 🐸

include/dev/HallSensor.hpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
@chl1043 chl1043 requested review from mjmagee991 and tmb5932 October 3, 2024 23:18
Copy link

@mjh9585 mjh9585 left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a few changes. Please remove the cmake-build-debug directory and make sure your build directory is set to 'build' in the CMAKE options. Also please update the README to describe the purpose of this repository. Also include the target device and the name of the main board target (REV3-WSS).

src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
include/dev/HallSensor.hpp Outdated Show resolved Hide resolved
targets/REV3-WSS/main.cpp Outdated Show resolved Hide resolved
targets/hall_sensor/main.cpp Show resolved Hide resolved
include/dev/HallSensor.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
Copy link

@mjmagee991 mjmagee991 left a comment

Choose a reason for hiding this comment

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

A lot of nitpicks, but the overall structure is pretty good.

Also, be sure to test this thoroughly. This driver is written assuming that we'll be able to poll the GPIO fast enough not to have issues, but that's not a great assumption to be making. If this isn't working, we may have to investigate an interrupt-based solution

include/WSS.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
targets/REV3-WSS/main.cpp Outdated Show resolved Hide resolved
targets/hall_sensor/main.cpp Outdated Show resolved Hide resolved
targets/hall_sensor/main.cpp Outdated Show resolved Hide resolved
@chl1043 chl1043 requested a review from mjmagee991 November 16, 2024 16:46
Copy link

@mjh9585 mjh9585 left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a few small things. Also don't forget to go through the unresolved comments and mark them. Once you have all of that, lets do one final test before merging.

CMakeLists.txt Outdated Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/dev/HallSensor.hpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/WSS.cpp Outdated Show resolved Hide resolved
targets/REV3-WSS/main.cpp Outdated Show resolved Hide resolved
Copy link

@mjmagee991 mjmagee991 left a comment

Choose a reason for hiding this comment

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

Looks much better than it did before, but there's a little more cleanup to do

include/WSS.hpp Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/WSS.hpp Outdated Show resolved Hide resolved
include/dev/HallSensor.hpp Show resolved Hide resolved
include/dev/HallSensor.hpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
src/dev/HallSensor.cpp Show resolved Hide resolved
src/dev/HallSensor.cpp Outdated Show resolved Hide resolved
targets/REV3-WSS/main.cpp Outdated Show resolved Hide resolved
@chl1043 chl1043 requested a review from mjmagee991 November 23, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants