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

Reading the Balance port (if present) #49

Merged
merged 26 commits into from
Mar 10, 2024
Merged

Reading the Balance port (if present) #49

merged 26 commits into from
Mar 10, 2024

Conversation

markmellors
Copy link
Contributor

seems to work well. gives a useful message if there's a problem. Currently the program doesn't change behaviour if there's a problem though.

Copy link
Collaborator

@robberwick robberwick left a comment

Choose a reason for hiding this comment

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

looks good. I think it's worth extracting the ADS1x15 library out into it's own thing, and ther are a couple of other very minor comments

@@ -14,6 +14,9 @@ namespace CONFIG {

#define CURRENT_CHALLENGE ESCAPE_ROUTE

constexpr int I2C_SDA_PIN = 20;
Copy link
Collaborator

Choose a reason for hiding this comment

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

pimoroni/pimoroni-pico#890 is merged into pimoroni-pico:main now - i wonder if we can use the MOTOR2040 namespace config values now? Assuming that they're compatible with the pins on our board of course...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

#include <string>
#include "pico/stdlib.h"
#include "hardware/i2c.h"
#include "ads1x15.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

it might be a bit of a stretch goal, but having a balance port library that isn't tightly coupled to the ADC hardware would be useful, as it would mean that it was portable between projects. It would also mean that we had a portable ADS1x15 library to use on other RP2040 projects for reasons other than specifically checking the balance port. realistically that would mean seperating the ADS1x15 and the balance_port libraries, and probably some kind of adaptor pattern interface so that different ADC drivers could be used. Probably not critical to go the whole hog for now, but worth at least doing the library separation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ADS1x15 is now a seperate (external) library, but the balance_port library is still tightly coupled to it.

@@ -36,6 +36,6 @@ ReceiverChannelValues ReceiverCPPM::get_channel_values() const {
}

bool ReceiverCPPM::get_receiver_data() const {
return decoder->getFrameAgeMs() > 0;
return decoder->getFrameAgeUs() > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this change related to the balance port checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but it was needed to get my code to compile. This change is now on main. I'd cherrypicked the commit from #48 , so expected this change to disappear with a rebase. I've now rebased, but it's made it worse, we now appear to have two commits making this change! I'm confused.

Copy link
Collaborator

@robberwick robberwick left a comment

Choose a reason for hiding this comment

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

Yes 👍

@robberwick robberwick merged commit 0233fcc into main Mar 10, 2024
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.

2 participants