This is a list of all comments for RO-LEAH-273-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeGenPermeate.c Revision Comment by Sean Nash on 04 March 2025, 14:57 https://devapps.diality.us/cru/RO-LEAH-273-1#c21636 ModeGenPermeate.c. Reply by Michael Garthwaite on 05 March 2025, 08:22 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:57 https://devapps.diality.us/cru/RO-LEAH-273-1#c21637 Add blank line between banner and #define. Reply by Michael Garthwaite on 05 March 2025, 08:22 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:52 https://devapps.diality.us/cru/RO-LEAH-273-1#c21675 Remove extra space between U32 and var name. That will also align comment. Reply by Michael Garthwaite on 11 March 2025, 09:17 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:58 https://devapps.diality.us/cru/RO-LEAH-273-1#c21639 Align "=". Reply by Michael Garthwaite on 05 March 2025, 08:22 > Fixed. Thanks! Revision Comment by Daniel Ho on 04 March 2025, 14:57 https://devapps.diality.us/cru/RO-LEAH-273-1#c21638 What is the time unit per count (e.g. minute, second, milliseconds, etc.)? Reply by Michael Garthwaite on 05 March 2025, 08:22 > its not a count per time but a count of general task ticks. > Its used to determine when we hit the interval period. Reply by Sean Nash on 05 March 2025, 08:33 > And the general task tick time is 50ms (20 Hz). Revision Comment by Sean Nash on 04 March 2025, 15:00 https://devapps.diality.us/cru/RO-LEAH-273-1#c21640 Use genPermeateState variable here instead of enum. Reply by Michael Garthwaite on 05 March 2025, 08:23 > Fixed. Thanks! Revision Comment by Daniel Ho on 04 March 2025, 15:03 https://devapps.diality.us/cru/RO-LEAH-273-1#c21644 Why not default case? Revision Comment by Daniel Ho on 04 March 2025, 14:56 https://devapps.diality.us/cru/RO-LEAH-273-1#c21635 Why need comment here? Revision Comment by Sean Nash on 05 March 2025, 08:34 https://devapps.diality.us/cru/RO-LEAH-273-1#c21667 Remove blank line. Reply by Michael Garthwaite on 11 March 2025, 09:17 > Fixed. Thanks! Revision Comment by Sean Nash on 11 March 2025, 09:28 https://devapps.diality.us/cru/RO-LEAH-273-1#c21724 Add blank line between break and default. Reply by Michael Garthwaite on 13 March 2025, 14:33 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 15:01 https://devapps.diality.us/cru/RO-LEAH-273-1#c21641 Are these TBDs? You'll set actuators as appropriate for each state? Revision Comment by Vinayakam Mani on 17 March 2025, 10:09 https://devapps.diality.us/cru/RO-LEAH-273-1#c21755 Looks this timer is being not used in low state timeout. Reply by Michael Garthwaite on 25 March 2025, 09:28 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 15:02 https://devapps.diality.us/cru/RO-LEAH-273-1#c21643 Shouldn't state get assigned to low or high state from here? If not, how do we get out of start state. Also, do we even need a start state? Revision Comment by Sean Nash on 06 March 2025, 13:57 https://devapps.diality.us/cru/RO-LEAH-273-1#c21679 We would be returning the "next" state, not the current state. Reply by Michael Garthwaite on 11 March 2025, 09:17 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:55 https://devapps.diality.us/cru/RO-LEAH-273-1#c21676 These next 2 possibilities look to be handled the same. Can we combine the else if condition? Reply by Michael Garthwaite on 11 March 2025, 09:17 > Fixed. Thanks! Revision Comment by Vinayakam Mani on 17 March 2025, 10:05 https://devapps.diality.us/cru/RO-LEAH-273-1#c21754 Are we using this flowrate here? it seems only level is being used for state transition. Reply by Michael Garthwaite on 25 March 2025, 09:28 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:56 https://devapps.diality.us/cru/RO-LEAH-273-1#c21677 I believe Jenna wanted an alarm for this situation - though you may want a bit of persistence before you trigger it. I see you have these else conditions in all 3 states. I think we can check for this once in a check function that gets called from the exec no matter which state we're in. Reply by Michael Garthwaite on 11 March 2025, 09:18 > We're gonna check for illegal state in the exec function > instead of each state function. Revision Comment by Sean Nash on 06 March 2025, 13:56 https://devapps.diality.us/cru/RO-LEAH-273-1#c21678 Add blank line before return statement. Reply by Michael Garthwaite on 11 March 2025, 09:18 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:58 https://devapps.diality.us/cru/RO-LEAH-273-1#c21680 Not initial. Low state. Reply by Michael Garthwaite on 11 March 2025, 09:18 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:58 https://devapps.diality.us/cru/RO-LEAH-273-1#c21681 stateDelayTime is at least one input. Reply by Michael Garthwaite on 11 March 2025, 09:18 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:59 https://devapps.diality.us/cru/RO-LEAH-273-1#c21682 Next state, not current state. Reply by Michael Garthwaite on 11 March 2025, 09:18 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:59 https://devapps.diality.us/cru/RO-LEAH-273-1#c21683 I don't think this state should have a delay. When we see high level, we should switch immediately. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:00 https://devapps.diality.us/cru/RO-LEAH-273-1#c21684 Add spaces between () and params (throughout). Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:01 https://devapps.diality.us/cru/RO-LEAH-273-1#c21685 If not doing anything for these conditions, add a comment at least saying we're not doing anyting so reader knows it's intentional and not an oversight. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:04 https://devapps.diality.us/cru/RO-LEAH-273-1#c21686 Not necessary if you check for this in exec as I recommended in comment above. Reply by Michael Garthwaite on 11 March 2025, 09:19 > We're gonna check for illegal state in the exec function > instead of each state function. Revision Comment by Sean Nash on 06 March 2025, 14:05 https://devapps.diality.us/cru/RO-LEAH-273-1#c21687 Add blank line before return statement. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:05 https://devapps.diality.us/cru/RO-LEAH-273-1#c21688 Not initial state. Full state. stateDelayTime is at least one input (not none). Returning "next" state, not current state. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:11 https://devapps.diality.us/cru/RO-LEAH-273-1#c21690 This should be >= medium && timeout. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 11 March 2025, 09:38 https://devapps.diality.us/cru/RO-LEAH-273-1#c21728 Swap level and medium to get logic right. We want level >= LEVEL_STATE_MEDIUM. Reply by Michael Garthwaite on 13 March 2025, 14:33 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:13 https://devapps.diality.us/cru/RO-LEAH-273-1#c21691 High state should be handled in above condition and removed here. Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:13 https://devapps.diality.us/cru/RO-LEAH-273-1#c21692 Low state should be checked first since it has a safety implication and the above condition could prevent it for timeout period. If low, we want to go to low state immediately (don't wait for timeout). Reply by Michael Garthwaite on 11 March 2025, 09:19 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:16 https://devapps.diality.us/cru/RO-LEAH-273-1#c21693 Not necessary to check here if you check it globally in exec. Reply by Michael Garthwaite on 11 March 2025, 09:20 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 14:17 https://devapps.diality.us/cru/RO-LEAH-273-1#c21694 Add blank line before return statement. Reply by Michael Garthwaite on 11 March 2025, 09:20 > Fixed. Thanks! ---------------------------------------- File: firmware/App/Modes/ModeGenPermeate.h Revision Comment by Sean Nash on 04 March 2025, 14:53 https://devapps.diality.us/cru/RO-LEAH-273-1#c21633 Fix misspelling. Reply by Michael Garthwaite on 13 March 2025, 14:33 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:55 https://devapps.diality.us/cru/RO-LEAH-273-1#c21634 I think p25Level and p16FlowRate are already being published in their respective monitor units. We shouldn't duplicate them here. Reply by Michael Garthwaite on 05 March 2025, 08:30 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 15:03 https://devapps.diality.us/cru/RO-LEAH-273-1#c21645 What is difference between water delivery in progress and gen permeate state? Reply by Michael Garthwaite on 05 March 2025, 08:30 > Based on the discussion we had earlier yesterday, i dont need > that variable as the DD will be managing it and this mode is > only monitoring the level sensors. Revision Comment by Daniel Ho on 04 March 2025, 14:38 https://devapps.diality.us/cru/RO-LEAH-273-1#c21626 The function name is not the same as the comment. Which one (to preprare or to transition)? Reply by Michael Garthwaite on 05 March 2025, 08:31 > Fixed. Thanks! ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Sean Nash on 04 March 2025, 14:45 https://devapps.diality.us/cru/RO-LEAH-273-1#c21627 Keep include lists in alphabetical order. Reply by Michael Garthwaite on 05 March 2025, 08:29 > I think thats in alphabetical? Revision Comment by Sean Nash on 11 March 2025, 09:43 https://devapps.diality.us/cru/RO-LEAH-273-1#c21732 Remove blank line. Revision Comment by Sean Nash on 04 March 2025, 14:47 https://devapps.diality.us/cru/RO-LEAH-273-1#c21628 In RODefs.h, you have a standby pause state. Not sure why you think you'll need that, but if it is needed it should be here. Reply by Michael Garthwaite on 05 March 2025, 08:29 > Based on previous comment regarding pause state, Im not > entire sure if i need it just yet. may remove or flesh out > implementation Revision Comment by Sean Nash on 04 March 2025, 14:48 https://devapps.diality.us/cru/RO-LEAH-273-1#c21629 Does this flag need to be overridable? If Dialin wants to kick off gen permeate mode, it should just proxy the DD command instead of sending an override. Reply by Michael Garthwaite on 11 March 2025, 09:38 > changed the variable to no longer be overridable. Removed > override function. Revision Comment by Vinayakam Mani on 17 March 2025, 10:20 https://devapps.diality.us/cru/RO-LEAH-273-1#c21756 Please add parentheses for each condition for better readability. Reply by Michael Garthwaite on 25 March 2025, 09:28 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:51 https://devapps.diality.us/cru/RO-LEAH-273-1#c21630 Should also verify we are even in standby mode before accepting this request. Reply by Michael Garthwaite on 11 March 2025, 09:38 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:51 https://devapps.diality.us/cru/RO-LEAH-273-1#c21631 Add extra (should be 2) blank line before test banner. Reply by Michael Garthwaite on 11 March 2025, 09:38 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:52 https://devapps.diality.us/cru/RO-LEAH-273-1#c21632 See comment above - is this function necessary? Reply by Michael Garthwaite on 11 March 2025, 09:39 > Removed. ---------------------------------------- File: firmware/App/Modes/ModeStandby.h Revision Comment by Sean Nash on 04 March 2025, 14:38 https://devapps.diality.us/cru/RO-LEAH-273-1#c21625 Add comment or remove other comments for consistency. Also, I've been naming this kind of function with prefix "signal" instead of "request" when a command is coming from another sub-system into the state machine. Reply by Michael Garthwaite on 05 March 2025, 08:24 > Fixed naming and added comment. Unsure at the moment if this > should be a public function in DD Interface or stay here. > i'll find out in testing. Reply by Sean Nash on 05 March 2025, 08:36 > I think it makes sense for signal functions to be in the > modes like it is now. It could be that the signal function > is called from DD interface though. ---------------------------------------- File: firmware/App/Modes/OperationModes.c Revision Comment by Sean Nash on 04 March 2025, 14:30 https://devapps.diality.us/cru/RO-LEAH-273-1#c21623 Need to add a column for RO_MODE_GENP too (not just a row). The row shows which modes (columns) you can transition to from that mode associated with the row. If it's allowed, put the new mode. If it's not allowed, put RO_MODE_NLEG. The items on the diagonal (top left to bottom right) are always allowed since you're not changing the mode. Reply by Michael Garthwaite on 05 March 2025, 08:24 > added column. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:31 https://devapps.diality.us/cru/RO-LEAH-273-1#c21624 Transition from GENP to service should be not legal (2nd column). Reply by Michael Garthwaite on 05 March 2025, 08:24 > Fixed. Thanks! ---------------------------------------- File: firmware/App/Services/DDInterface.c Revision Comment by Sean Nash on 06 March 2025, 13:50 https://devapps.diality.us/cru/RO-LEAH-273-1#c21674 Include ROPump instead and add abstraction layer functions there as needed. Revision Comment by Sean Nash on 04 March 2025, 14:29 https://devapps.diality.us/cru/RO-LEAH-273-1#c21622 DDInterface Revision Comment by Sean Nash on 04 March 2025, 14:23 https://devapps.diality.us/cru/RO-LEAH-273-1#c21620 Remove blank line. Revision Comment by Vinayakam Mani on 17 March 2025, 10:24 https://devapps.diality.us/cru/RO-LEAH-273-1#c21757 function header says return type as void. Reply by Michael Garthwaite on 25 March 2025, 09:28 > Fixed. Thanks! Revision Comment by Sean Nash on 25 March 2025, 09:41 https://devapps.diality.us/cru/RO-LEAH-273-1#c21832 If monitoring not yet implemented, that's ok - but at least add a TODO comment. Reply by Michael Garthwaite on 25 March 2025, 09:45 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:45 https://devapps.diality.us/cru/RO-LEAH-273-1#c21671 I feel like this unit should not be calling a driver level function directly. There should be a layer in ROPump.c between the driver and high level units like DDInterface. Reply by Michael Garthwaite on 13 March 2025, 14:34 > Will be implemented with RO closed loop control. Revision Comment by Sean Nash on 06 March 2025, 13:46 https://devapps.diality.us/cru/RO-LEAH-273-1#c21672 Remove blank line. Reply by Michael Garthwaite on 13 March 2025, 14:34 > Fixed. Thanks! Revision Comment by Sean Nash on 06 March 2025, 13:47 https://devapps.diality.us/cru/RO-LEAH-273-1#c21673 Add spaces between () and params (throughout this unit I see several). Also throughout, spaces between params. Revision Comment by Sean Nash on 04 March 2025, 14:27 https://devapps.diality.us/cru/RO-LEAH-273-1#c21621 I believe this is already called by caller to this function (handleIncomingMessage in Messaging.c) when it returns, so this is redundant. Reply by Michael Garthwaite on 05 March 2025, 08:25 > Fixed. Thanks! ---------------------------------------- File: firmware/App/Services/DDInterface.h Revision Comment by Sean Nash on 04 March 2025, 14:23 https://devapps.diality.us/cru/RO-LEAH-273-1#c21619 DDInterface.h Reply by Michael Garthwaite on 11 March 2025, 09:23 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:22 https://devapps.diality.us/cru/RO-LEAH-273-1#c21618 Who would call this function? DD is calling handle function below, not this one. I think that handle function is only one calling this, so should be private (static). Reply by Michael Garthwaite on 05 March 2025, 08:26 > Originally i was thinking i would need it public for > overrides and/or proxy commands. May change it to private > once i implement them. Revision Comment by Sean Nash on 05 March 2025, 08:42 https://devapps.diality.us/cru/RO-LEAH-273-1#c21670 Remove extra blank line. Reply by Michael Garthwaite on 11 March 2025, 09:24 > Fixed. Thanks! ---------------------------------------- File: firmware/App/Services/Messaging.c Revision Comment by Sean Nash on 04 March 2025, 14:20 https://devapps.diality.us/cru/RO-LEAH-273-1#c21617 Why does this need to be public? Reply by Michael Garthwaite on 05 March 2025, 08:25 > Fixed based on comment in DD interface. Changed back to > private. ---------------------------------------- File: firmware/App/Services/Messaging.h Revision Comment by Sean Nash on 05 March 2025, 08:39 https://devapps.diality.us/cru/RO-LEAH-273-1#c21669 Remove extra blank line. Reply by Michael Garthwaite on 11 March 2025, 09:23 > Fixed. Thanks! ---------------------------------------- File: RODefs.h Revision Comment by Sean Nash on 04 March 2025, 14:05 https://devapps.diality.us/cru/RO-LEAH-273-1#c21614 Add comment. Also, we've customarily kept mode abbreviation to 4 letters (i.e. GENP). Reply by Michael Garthwaite on 05 March 2025, 08:17 > Fixed. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:06 https://devapps.diality.us/cru/RO-LEAH-273-1#c21615 RO (or FP), not DD. Reply by Michael Garthwaite on 05 March 2025, 08:17 > Fixed To RO. Thanks! Revision Comment by Sean Nash on 04 March 2025, 14:06 https://devapps.diality.us/cru/RO-LEAH-273-1#c21616 What is "idle" state doing that needs to be paused? Reply by Michael Garthwaite on 05 March 2025, 08:17 > Its probably not necessary. i want it as a fallback state for > alarms when testing later this week instead of faulting where > i can. I'll probably remove it once testing is complete. Reply by Sean Nash on 05 March 2025, 08:30 > If standby idle isn't doing anything, why not stay in idle > during alarms? --- ID: RO-LEAH-273-1 https://devapps.diality.us/cru/RO-LEAH-273-1 Title: RO-LEAH-273_FW RO Generate Water Mode Statement of Objectives: State: Closed Summary: Author: Michael Garthwaite Moderator: Michael Garthwaite Reviewers: (7 active, 0 completed*) Sean Nash Tiffany Mejia Vinayakam Mani Dara Navaei amanesh Behrouz NematiPour Daniel Ho