-
Notifications
You must be signed in to change notification settings - Fork 0
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
Food on Fork Integration with Web App #76
Conversation
Note: This is not fully complete. I am hoping to get some feedback towards my approach through this draft. I understand that there aren't testing methods outlined yet. I will be doing that in a later draft. Additionally, I will be adding description in a later draft as well. Here is my current implementation:
Again, I am mainly looking to get thoughts on what you think of an approach like this. In addition to this, I am thinking the trigger for being able to subscribe is currently useEffect() of rendering the Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See in-line comments. In addition, below are comments on your overall algorithm.
- Waiting 1 minute to detect whether food is on the fork is WAY too long, so is 10 seconds. If the automated food-on-fork detection is not faster than it would take users to click the button themselves, they will just do that. Your node runs at 30 Hz; why do we need to wait that long to decide whether there is food on the fork or not?
- Why did you add a throttle rate to the subscription to the ROS topic? Every time we get a new reading from the food on fork node, we should record that. By taking one message every second , you are dropping 29/30 of all the messages...
- I assume that the reason you want to wait some time period and then take the average is to avoid noise. But remember what I said about starting with simple solutions first and then only moving to more complex solutions if they are motivated?
- You should start with a system that gets every message published by the node (i.e., no throttle), and if that message is lower than the threshold, immediately move above plate.
- And then test that out to understand its strengths and weaknesses.
- Only if noise is truly an issue should we move to an approach that averages readings over a window.
- Further, even as we move to move complex solutions, we have to be very data-driven — why do you need to wait 60 seconds to average the readings? Why not wait 0.3 secs? Every design decision you make when designing an algorithm must be justified by the data/domain. (e.g., measure the nosie you get, see how frequently you see that noise, and use that interval to determine an amount of time to average over)
- What do you mean “we are only able to call one function to move back above the plate?” Why can’t re-acquire bite and your subscriber callback both call the same function? That way, if the user decides to move the robot above the plate before your automated system does (or your automated system is not in the threshold where it is making a decision), the user can click the button.
feedingwebapp/src/Pages/Constants.js
Outdated
@@ -104,3 +106,8 @@ export const ROS_ACTION_STATUS_CANCEL_GOAL = '2' | |||
export const ROS_ACTION_STATUS_SUCCEED = '3' | |||
export const ROS_ACTION_STATUS_ABORT = '4' | |||
export const ROS_ACTION_STATUS_CANCELED = '5' | |||
|
|||
/** | |||
* Constant range of probability values that defines Food on Fork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe how your node interprets the probabilities.
1000 | ||
) | ||
|
||
return () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return function gets called before useEffect gets called again. Why should we wait until them before checking whether the probability is in the range? This check should be in the subscriber's callback function.
ros.current, | ||
FOOD_ON_FORK_TOPIC.name, | ||
FOOD_ON_FORK_TOPIC.type, | ||
(message) => setFoodProb((prevVal) => [...prevVal, Number(message.data)]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a separate function (use the useCallback
hook), because, as I suggested below, you will be adding more lines to this function. Imo, multi-line functions are more readable when defined as separate functions, not in-line.
function TestROSSubscribe() { | ||
function TestROSSubscribe(props) { | ||
// get the value of props | ||
let topicType = props.topicType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good generalization of this component!
moveAbovePlate() | ||
} | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read the documentation for useEffect
, specifically around the dependencies list. As I understand it, without dependencies, useEffect
will be called every time a re-render happens. So every time a re-render happens you are unsubscribing and re-subscribing from the topic, which is extremely wasteful.
You should instead only subscribe to the topic once when this component starts. Because you pass a callback function to the subscriber, that callback function will be called every time a new message is received---you don't need to re-enter to useEffect
call for this. And then, the return function from useEffect
should unsubscribe from the topic, but that will only be called once when the component unmounts.
To achieve the above functionality of useEffect being called only once, you need to either pass in an empty list as dependencies, or all dependencies must also never change during the lifetime of this component. Reading into the documentation of each hook will help you understand when each one gets called, which will help you understand which variables are and aren't changing when.
@amalnanavati A followup to your bullet point 4: What I mean by only function is the following: |
You are correct that "move to resting position" can only be initiated by the user, whereas "move above plate" can be initiated by the user or by your algorithm. |
BTW I'm confused by which page you are focusing on. In your comment, you wrote "When the web app reaches the bite done screen, we see two buttons (bite done and re-acquire bite)." On the "Bite Done" page, the buttons are "Bite Done" and "Take Another Bite." Did you mean "take another bite" instead of "re-acquire bite"? ("re-acquire bite" appears on the BiteAcquisitionCheck page) |
@amalnanavati yikes, yes! I meant take another bite and not reacquire bite. |
Closed by #127 |
Describe this pull request. Link to relevant GitHub issues, if any.
[TODO]
Explain how this pull request was tested, including but not limited to the below checkmarks.
[TODO]
Before creating a pull request
npm run format
python3 -m black .
in the top-level of this repositoryBefore merging a pull request
Squash and Merge
)