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.

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

Feature/AzureRtos #177

Merged
merged 3 commits into from
Apr 26, 2021
Merged

Feature/AzureRtos #177

merged 3 commits into from
Apr 26, 2021

Conversation

elsalahy
Copy link
Contributor

Summary:

This PR adds support for the awesome AzureRTOS (threadx)
Closes #165

This paves the way for the LoRaWAN example in #166

Changes:

  • Adds threadx submodule
  • Adds a basic_azurertos application

Notes for Reviewers:

...

Release Notes: (optional)

Add AzureRTOS support

@elsalahy elsalahy added the s/apps Application related development label Apr 21, 2021
@elsalahy elsalahy added this to the 2021 Q2 milestone Apr 21, 2021
@elsalahy elsalahy self-assigned this Apr 21, 2021
@elsalahy
Copy link
Contributor Author

@johanstokking feel free to give it a quick look, but this is mainly just a notification for you to be aware of the new support.

mcserved
mcserved previously approved these changes Apr 22, 2021
Copy link
Contributor

@mcserved mcserved left a comment

Choose a reason for hiding this comment

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

I have a few minor things you might want to look at, but overall the application works (I even got it to replace the led with a buzzer) and there were no real deal breakers I could find. The power consumption of the base app is kinda awful (20mA all the time), but I have hope that the lorawan version will improve upon this :)

void thread_1_entry(ULONG thread_input);
void thread_2_entry(ULONG thread_input);

int main(int argc, char **argv)
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 reason this particular app uses argc/argv instead of void like all the other apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's inherited from some demo code, good catch

tx_kernel_enter();
}

/* Define what the initial system looks like */
Copy link
Contributor

Choose a reason for hiding this comment

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

Extremely nitpicky, but this comment (and some others in this file) end with two spaces instead of one before closing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this

/* The length of time the LED will remain on for. It is on just long enough
to be able to see with the human eye so as not to distort the power readings too
much. */
#define APP_LED_TOGGLE_DELAY (20)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extremely nitpicky, but basic_freertos has essentially the same config variable but only it's called LED_TOGGLE_DELAY, so maybe for consistency we could change this or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this but in the future all app conf will have an APP prefix

}
else
{
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it wise to break out of the while loop if the queue sending fails? What will happen when a user accidentally configures it wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is intentional, because if there is bad configuration, the app should not function

mcserved
mcserved previously approved these changes Apr 23, 2021
Copy link
Contributor

@mcserved mcserved left a comment

Choose a reason for hiding this comment

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

App still builds and works

lib: Adjust system clock t0 48Mhz


doc: Fix some spelling mistakes in lib description
app: Improve basic_azurertos app comments


doc: Add basic_azurertos config link


dev: Add basic_azurertos STM32CubeIDE support
@elsalahy
Copy link
Contributor Author

@marnixcro I reduced the number of commits and did some minor docs improvements, please give it an approve when you can.

Copy link
Contributor

@mcserved mcserved left a comment

Choose a reason for hiding this comment

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

Cmake and STM32CubeIDE still build

@elsalahy elsalahy merged commit f872a76 into develop Apr 26, 2021
@elsalahy elsalahy deleted the feature/azure_rtos branch April 26, 2021 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s/apps Application related development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Azure RTOS support (basic application)
2 participants