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

Add support for sensor polling at a specific frequency #38

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for sensor polling at a specific frequency #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6338820
Update sensor.cpp
allosaurus7000 Nov 22, 2020
7ff2a80
Update sensor.h
allosaurus7000 Nov 22, 2020
f5a1f70
Update sensor.h
allosaurus7000 Nov 22, 2020
5d8b11c
Update sensor.cpp
allosaurus7000 Nov 22, 2020
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
8 changes: 7 additions & 1 deletion Software/spartan/src/sensors/sensor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
#include "sensor.h"

#include "utilis.h"
// Constructor
Comment on lines +2 to 3
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

spartan::Sensor::Sensor(int busID, int instance)
: m_busID(busID), m_status(STATUS_OFF), m_instance(instance) {}

unsigned long spartan::Sensor::poll_correct_frequency() {
if (getTimeMillis() > last_polled + frequency) {
last_polled = getTimeMillis();
}
}
Comment on lines +7 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// Debug output
void spartan::Sensor::printEscapedValues(bool normalize) const {
int lines = printValues();
Expand Down
4 changes: 4 additions & 0 deletions Software/spartan/src/sensors/sensor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace spartan {
// Standard sensor implementation
virtual int powerOn() = 0;
virtual int powerOff() = 0;
virtual long last_polled() = 0;
virtual unsigned long poll_correct_frequency();
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

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

virtual int poll(MasterDataPacket &dp) = 0;

// Debug options
Expand All @@ -29,6 +31,7 @@ namespace spartan {
// Data getters
int getBusID() const;
int getInstance() const;
const long frequency = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Return status (operate with interfaced constants described in globals.h)
virtual int getStatus() const;

Expand All @@ -40,3 +43,4 @@ namespace spartan {
} // namespace spartan

#endif // SENSOR_H_INCLUDED