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.

Sign up for GitHub

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

Jump to bottom

Make brushed motors resistant to changing missions #474

Merged
merged 11 commits into from
May 12, 2023
Merged

Make brushed motors resistant to changing missions #474

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c0de872
isLive bool changed to a struct
wisabel0 May 2, 2023
f5429e7
global map that keeps track of all isLive states
wisabel0 May 2, 2023
8bef481
made changes from draft PR, but kept inline static mutex
wisabel0 May 3, 2023
20dfdfd
added commeents for bit shifting, changed scope of unique lock, and a…
wisabel0 May 3, 2023
e32f773
Merge branch 'master' into isabel/makeLive
wisabel0 May 3, 2023
2e4f52b
switched to single mutex and made the map entry a string for joint name
wisabel0 May 8, 2023
833cece
cleaned up comments, moved mutex construction
wisabel0 May 8, 2023
eadd2a9
Merge branch 'master' into isabel/makeLive
wisabel0 May 9, 2023
f3b42f3
Fix merge conflicts
tabiosg May 10, 2023
676ee68
Fix comment
tabiosg May 10, 2023
38d2197
Remove outdated code
tabiosg May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions src/esw/brushed_motors/Controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Controller::Controller(
currentAngle = 0.0f;
}


// REQUIRES: nothing
// MODIFIES: nothing
// EFFECTS: Returns last saved value of angle.
Expand Down Expand Up @@ -61,9 +62,9 @@ void Controller::overrideCurrentAngle(float newAngleRad) {
// REQUIRES: nothing
// MODIFIES: nothing
// EFFECTS: Returns true if Controller is live.
bool Controller::isControllerLive() const {
return isLive;
}
// bool Controller::isControllerLive() const {
// return isLive;
// }

// REQUIRES: -1.0 <= input <= 1.0
// MODIFIES: currentAngle. Also makes controller live if not already.
Expand Down Expand Up @@ -197,12 +198,45 @@ void Controller::turnOn() const {
}

// REQUIRES: nothing
// MODIFIES: isLive
// MODIFIES: nothing
// EFFECTS: Returns a combined ID for both the deviceAddress and motorID
uint8_t Controller::combineDeviceMotorID() const {
return (deviceAddress << 3) | motorID;
tabiosg marked this conversation as resolved.
Show resolved Hide resolved
}

// REQUIRES: nothing
// MODIFIES: liveMap
// EFFECTS: I2C bus, if not already live,
// configures the physical controller.
// Then makes live.

std::unordered_map<uint8_t, LiveState> Controller::liveMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if you ran into this issue when emplacing but if you run into issues when emplacing, it's probably because C++ does not know how to handle default stuff when it is a uint8_t (it requires a hash). An easy and acceptable fix is to just make the unordered map take an integer fr the key


void Controller::makeLive() {
if (isLive) {
// if (isLive) {
// return;
// }

uint8_t key = combineDeviceMotorID();

// if key is absent, no motor with that key has been made live
auto it = liveMap.find(key);
if (it != liveMap.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wait I think you'll want to put the unique lock outside of the if statement, I think it loses scope right after it leaves the if statement. So I think you can find a way to move it outside the if statement (the if statement is only necessary for emplacing the key,state into the liveMap

std::unique_lock<std::mutex>
lck(it->second.liveMutex);
// it->second.liveMutex.lock();
} else {
LiveState state;
state.jointName = name;
liveMap.emplace(key, state);
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use a lock guard whenever emplacing (basically just need to put a lock and unlock around the emplace, but a lock_guard can do that.(something like std::lock_guardstd::mutex lock(state.liveMutex); )

// liveMap[key] = state; // doesn't work anymore because mutex isn't inline static
std::unique_lock<std::mutex>
lck(state.liveMutex);
// state.liveMutex.lock();
}

// already live and configured to correct motor
tabiosg marked this conversation as resolved.
Show resolved Hide resolved
if (it->second.jointName == name) {
return;
}

Expand Down Expand Up @@ -257,7 +291,10 @@ void Controller::makeLive() {
I2C::transact(deviceAddress, motorIDRegMask | LIMIT_A_IS_FWD_OP, LIMIT_A_IS_FWD_WB,
LIMIT_A_IS_FWD_RB, buffer, nullptr);

isLive = true;

// update liveMap
auto it = liveMap.find(key);
CameronTressler marked this conversation as resolved.
Show resolved Hide resolved
it->second.jointName = name;

} catch (IOFailure& e) {
ROS_ERROR("makeLive failed on %s", name.c_str());
Expand Down
18 changes: 17 additions & 1 deletion src/esw/brushed_motors/Controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ sent. This is to prevent multiple virtual Controller objects from
trying to contact the same physical Controller object.)
*/

struct LiveState {
std::string jointName;
inline static std::mutex liveMutex;
CameronTressler marked this conversation as resolved.
Show resolved Hide resolved
};

class Controller {
public:
Expand Down Expand Up @@ -182,6 +186,11 @@ class Controller {
// EFFECTS: I2C bus, and turns on the controller. Can be used as a way to tick the watchdog for a particular mcu.
void turnOn() const;

// REQUIRES: nothing
// MODIFIES: nothing
// EFFECTS: Returns a combined ID for both the deviceAddress and motorID
uint8_t combineDeviceMotorID() const;


private:
// REQUIRES: nothing
Expand All @@ -205,6 +214,13 @@ class Controller {

float currentAngle;

bool isLive = false;
// bool isLive = false;

// key is deviceAddress and motorID (eg. if deviceAddress = 2(0b10) and motorID = 1(0b1), then key = 17(0b10001) )
static std::unordered_map<uint8_t, LiveState> liveMap;

bool isControllerCalibrated = false;


};