-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implementation of the offsets in the RateGroupDriver #2166
Conversation
@SMorettini upon initial inspection this looks good, thanks so much for (hopefully) taking it across the finish line |
@SMorettini you should make sure to update the RPI deployment as well. This is the source of the code break. Also, "No raw arrays in interfaces" is caused because you created an array of struct DivisderSet {
Divider dividers[Svc::RateGroupDriver::DIVIDER_SIZE];
} I'd prefer it if you'd make that adjustment as this was what we were trying to adjust last time. |
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.
Made some comments in the discussion. Let me know what you'd do and we'll pick-up the rest.
@LeStarch I added the changes requested, let me know if some other improvements are required. I also created a pull request to update the Led blinker tutorial to work with the new RateGroupDriver dividers: fprime-community/fprime-workshop-led-blinker#41 |
@@ -33,6 +33,24 @@ | |||
class RateGroupDriver : public RateGroupDriverComponentBase { | |||
|
|||
public: | |||
//! Size of the divider table, provided as a constants to users passing the table in | |||
static const NATIVE_UINT_TYPE DIVIDER_SIZE = NUM_CYCLEOUT_OUTPUT_PORTS; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
//! Divisor | ||
NATIVE_INT_TYPE divisor; | ||
//! Offset | ||
NATIVE_INT_TYPE offset; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
//! \brief Struct describing a divider | ||
struct Divider{ | ||
//! Divisor | ||
NATIVE_INT_TYPE divisor; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
I made some changes here:
Need to let CI run, but otherwise ready to go! |
//! Initializes divisor and offset to passed-in pair | ||
Divider(NATIVE_INT_TYPE divisorIn, NATIVE_INT_TYPE offsetIn) : | ||
divisor(divisorIn), offset(offsetIn) | ||
{} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
struct Divider{ | ||
//! Initializes divisor and offset to 0 (unused) | ||
Divider() : divisor(0), offset(0) | ||
{} |
Check notice
Code scanning / CodeQL
More than one statement per line Note
Divider() : divisor(0), offset(0) | ||
{} | ||
//! Initializes divisor and offset to passed-in pair | ||
Divider(NATIVE_INT_TYPE divisorIn, NATIVE_INT_TYPE offsetIn) : |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
Divider() : divisor(0), offset(0) | ||
{} | ||
//! Initializes divisor and offset to passed-in pair | ||
Divider(NATIVE_INT_TYPE divisorIn, NATIVE_INT_TYPE offsetIn) : |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
// copy provided array of dividers | ||
for (NATIVE_INT_TYPE entry = 0; entry < numDividers; entry++) { | ||
this->m_dividers[entry] = dividers[entry]; | ||
for (NATIVE_UINT_TYPE entry = 0; entry < RateGroupDriver::DIVIDER_SIZE; entry++) { |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
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.
Approve but with a general question about init() calls...
@thomas-bc: core component, would you be a second set of 👀 ? |
static const NATIVE_UINT_TYPE DIVIDER_SIZE = NUM_CYCLEOUT_OUTPUT_PORTS; | ||
|
||
//! has the configure method been called | ||
bool m_configured; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
|
||
// Loop through each divider. For a given port, the port will be called when the divider value | ||
// divides evenly into the number of ticks. For example, if the divider value for a port is 4, | ||
// it would be called every fourth invocation of the CycleIn port. | ||
for (NATIVE_INT_TYPE entry = 0; entry < this->m_numDividers; entry++) { | ||
if (this->m_dividers[entry] != 0) { | ||
for (NATIVE_UINT_TYPE entry = 0; entry < RateGroupDriver::DIVIDER_SIZE; entry++) { |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
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.
🚀
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.
Approved! Thanks for the fixes!
Co-authored-by: M Starch <[email protected]>
Co-authored-by: M Starch <[email protected]>
* Add support for new rate group dividers (#41) * Add support for new rate group dividers * Fix naming mistake * Fixing topology for nasa/fprime#2166 --------- Co-authored-by: Simone Morettini <[email protected]> Co-authored-by: M Starch <[email protected]>
Change Description
This pull request adds the possibility to define some offset in the RateGroupDriver. This pull request was first created here but never merged. An issue was reopened here.
I apply the changes suggested in the previous pull request with the suggested changes from the reviewer @LeStarch .