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

Add "Put" Actuator Data to turn on/off LED #3

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

Conversation

seojeongmoon
Copy link
Collaborator

No description provided.

@rosetree rosetree self-requested a review November 4, 2019 14:02
Copy link
Collaborator

@rosetree rosetree left a comment

Choose a reason for hiding this comment

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

Thanks @seojeongmoon for addressing this!

I leave some comments about this pull request. But more general: Could you update your code to meet the RIOT coding conventions? Like the usage of spaces, indentation and stuff.

Also I don’t really know, how I am supposed to use this feature. Can you explain the usage in a new section of README.md? How should the resource be called? What data should be submitted?


/* CoAP resources. Must be sorted by path (ASCII order). */
static const coap_resource_t _resources[] = {
{ "/hum", COAP_GET, _sense_hum_handler, NULL },
{ "/press", COAP_GET, _sense_press_handler, NULL },
{ "/saul/cnt", COAP_GET, _saul_cnt_handler, NULL },
{ "/saul/dev", COAP_POST, _saul_dev_handler, NULL },
{"/saul/atr", COAP_PUT, _saul_atr_handler, NULL},
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does atr stand for? Attribute? Another coding style related request: could you use a space after { and before }, like in the surrounding lines.

@@ -38,10 +38,21 @@ The idea of these resources is, to offer similar functionality as the


## Build and Execute

Directory: X/riot-saul-coap/RIOT/examples/gcoap/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a small statement when this directory should be used? We only need it for the GET requests, but still have to run the make command in the project directory. Also you could change X/riot-saul-coap to . (which would mean: in this project directory).


coap get <ip address> 5683 <resource>

With DTLS, port number is 5684
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea to include the different port! Thanks :)

@@ -13,7 +13,8 @@
* @file
* @brief CoAP endpoint for the SAUL registry
*
* @author Micha Rosenbaum <[email protected]>
* @author Seojeong Moon <[email protected]>
@author Micha Rosenbaum <[email protected]>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you, that you included yourself. I should have done this earlier.

Could need some code style improvements, like the * at the beginning and the spaces between the @author tag and the name.

saul_coap.c Outdated
@@ -24,30 +25,41 @@
#include "saul_reg.h"
#include "fmt.h"
#include "net/gcoap.h"
#include "cbor.h"
#include "phydat.h"
//#include "winch.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What’s that? Work in progress?

@@ -0,0 +1,78 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this file be merged?

rosetree pushed a commit that referenced this pull request Jan 28, 2020
update RIOT submodule to include DTLS patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants