Add support for sensor polling at a specific frequency #38

allosaurus7000 · 2020-11-22T08:53:57Z

Closes #29.

matthewcn56 · 2020-12-04T02:01:04Z

Current pull request right now has the frequency as a constant once per second, but we're supposed to make it so that each sensor can get polled at different rates right?

matthewcn56 · 2020-12-04T02:14:21Z

Would having a polling frequency being added into the constructor of Sensor make more sense so it can be variable for each sensor?

allosaurus7000 · 2020-12-07T22:10:51Z

Hi, sorry for the late response. I assumed it was supposed to be a universal polling frequency that can be called by any sensor. I set the const long frequency to 1000, which I think is supposed to be compared to the values in getMillis. I haven't gone back and fixed the code yet. The current code is calling the wrong boundary and is crashing. Feel free to make whatever corrections or changes that you see fit. Although we should clarify the first point with Kevin.

…

On Thu, Dec 3, 2020 at 6:14 PM Matthew Nieva ***@***.***> wrote: Would having a polling frequency being added into the constructor of Sensor make more sense so it can be variable for each sensor? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOJCHDZZNDB4IUZWSY6TV33STBAY5ANCNFSM4T6K5LXA> .

allosaurus7000 · 2020-12-07T22:13:04Z

I see that you have updated it with Kevin! Thanks so much for the changes. Hope everything is resolved now

…

On Mon, Dec 7, 2020 at 2:10 PM YuChen Chen ***@***.***> wrote: Hi, sorry for the late response. I assumed it was supposed to be a universal polling frequency that can be called by any sensor. I set the const long frequency to 1000, which I think is supposed to be compared to the values in getMillis. I haven't gone back and fixed the code yet. The current code is calling the wrong boundary and is crashing. Feel free to make whatever corrections or changes that you see fit. Although we should clarify the first point with Kevin. On Thu, Dec 3, 2020 at 6:14 PM Matthew Nieva ***@***.***> wrote: > Would having a polling frequency being added into the constructor of > Sensor make more sense so it can be variable for each sensor? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#38 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AOJCHDZZNDB4IUZWSY6TV33STBAY5ANCNFSM4T6K5LXA> > . >

harrisonCassar

A good start! See below:

harrisonCassar · 2021-01-11T21:44:15Z

Software/spartan/src/sensors/sensor.h

@@ -29,6 +31,7 @@ namespace spartan {
        // Data getters
        int getBusID() const;
        int getInstance() const;
+		const long frequency = 1000; 


Just a styling/lint issue: try to keep the indentation the same if possible!

harrisonCassar · 2021-01-11T21:44:54Z

Software/spartan/src/sensors/sensor.cpp

+#include "utilis.h"
 // Constructor


Just a styling/lint issue: adding a newline between the include directives and the rest of the source code helps keep the file more readable.

harrisonCassar · 2021-01-11T21:46:04Z

Software/spartan/src/sensors/sensor.h

+	virtual long last_polled() = 0; 
+	virtual unsigned long poll_correct_frequency(); 


Just a styling/lint issue: try to keep the indentation the same if possible!

harrisonCassar · 2021-01-11T21:57:09Z

Software/spartan/src/sensors/sensor.cpp

+unsigned long spartan::Sensor::poll_correct_frequency() {
+	if (getTimeMillis() > last_polled + frequency) {
+		last_polled = getTimeMillis(); 
+	}
+}


Based on what is shown here, I presume this method is meant to provide the Sensor child class with the ability to check if it should poll during this invocation by checking if enough time has passed since the last time it polled the sensor.

This is good! This is needed in some fashion in order to implement the functionality we desire: polling at different frequencies.

Currently, however, there's a few problems:

I don't seem to see the last_polled variable defined anywhere at all in the Sensor abstract based class... this should be resulting in a compilation/build error right now. Perhaps add a private member variable that holds that information? Maybe named m_last_poll or something along those lines?

The current name for this method poll_correct_frequency I don't think properly reflects the intent of the function in my eyes. If the method's intent is as what I outlined above, then I would think something like is_poll_ready... or something along those lines (I actually don't like my name here, so maybe something else 😄 ).

The use of frequency might be a bit ambiguous, as we are using it as a "period" of time instead of a "number of polls per unit of time". Perhaps change this name to something like polling_period?

This method doesn't actually return any value (needs to return of type unsigned long). This should be causing a compilation error.

harrisonCassar · 2021-01-11T22:25:58Z

Software/spartan/src/sensors/sensor.h

@@ -29,6 +31,7 @@ namespace spartan {
        // Data getters
        int getBusID() const;
        int getInstance() const;
+		const long frequency = 1000; 


Specifying this variable that represents the frequency/period of polling should likely not be something we want to be in the public access section of our base Sensor class. By doing so, we allow any other portion of the FSW outside of the class to freely modify this frequency for any of the instantiated Sensor child objects. This could be error-prone, as we expect only the Sensor class and (potentially) its derived classes to modify this frequency variable.

In other words, this is something that should only be handled by the internal methods of the Sensor base class, since we want to ensure that we're manipulating this frequency in an expected and safe manner. At this point, we expect this frequency to be defined by compile-time, and it should be used when instantiating a new object for a Sensor-derived class. If we want to support run-time frequency changes, we should only allow access of this private member variable through a protected "setter" method that can be exclusively accessed by Sensor and its derived classes.

Here's my quick thoughts:

Move this frequency variable from public to private.

Add some accessor functions (you might not even need these depending on how you implement/what we want), including a "getter" and a "setter". Let's just start with the "getter" as public, and the "setter" as a protected method (this will support potential dynamic frequencies in the future!).

Rename this frequency variable to more accurately represent the data it represents (refer to previous comment I made on this review).

harrisonCassar · 2021-01-11T22:33:00Z

Software/spartan/src/sensors/sensor.h

@@ -29,6 +31,7 @@ namespace spartan {
        // Data getters
        int getBusID() const;
        int getInstance() const;
+		const long frequency = 1000; 


Is there a particular reason why we have our "default" value as 1000 milliseconds?

When making some of the other changes I mentioned in this review, remember that we want our Sensor-derived classes to themselves define their own frequencies... using an initializer list for this would probably be the best option.

harrisonCassar

Also, the current changes seem to mostly aim to address the first part of what Issue #29's description mentions. Don't forget to take a look at the other portions too!

In summary, this is to properly update the main flight loop in main.cpp to actually poll the sensors at these desired frequencies, as well as updating the Sensor-derived classes to obey the new functionality/interface.

allosaurus7000 added 4 commits November 22, 2020 00:50

Update sensor.cpp

6338820

Update sensor.h

7ff2a80

Update sensor.h

f5a1f70

Update sensor.cpp

5d8b11c

harrisonCassar changed the title ~~Polling frequency updates~~ Add support for sensor polling at a specific frequency Jan 11, 2021

harrisonCassar requested changes Jan 11, 2021

View reviewed changes

harrisonCassar self-requested a review January 11, 2021 22:52

harrisonCassar reviewed Jan 11, 2021

View reviewed changes

harrisonCassar mentioned this pull request Jan 13, 2021

Changed Sensor Constructors to allow Variable Polling Frequency #39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sensor polling at a specific frequency #38

Add support for sensor polling at a specific frequency #38

allosaurus7000 commented Nov 22, 2020 •

edited by harrisonCassar

Loading

matthewcn56 commented Dec 4, 2020

matthewcn56 commented Dec 4, 2020

allosaurus7000 commented Dec 7, 2020 via email

allosaurus7000 commented Dec 7, 2020 via email

harrisonCassar left a comment

harrisonCassar Jan 11, 2021

harrisonCassar Jan 11, 2021

harrisonCassar Jan 11, 2021

harrisonCassar Jan 11, 2021

harrisonCassar Jan 11, 2021

harrisonCassar Jan 11, 2021

harrisonCassar left a comment

		virtual long last_polled() = 0;
		virtual unsigned long poll_correct_frequency();

Add support for sensor polling at a specific frequency #38

Are you sure you want to change the base?

Add support for sensor polling at a specific frequency #38

Conversation

allosaurus7000 commented Nov 22, 2020 • edited by harrisonCassar Loading

matthewcn56 commented Dec 4, 2020

matthewcn56 commented Dec 4, 2020

allosaurus7000 commented Dec 7, 2020 via email

allosaurus7000 commented Dec 7, 2020 via email

harrisonCassar left a comment

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar Jan 11, 2021

Choose a reason for hiding this comment

harrisonCassar left a comment

Choose a reason for hiding this comment

allosaurus7000 commented Nov 22, 2020 •

edited by harrisonCassar

Loading