-
Notifications
You must be signed in to change notification settings - Fork 12
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
rewrite of remote control code #5
base: master
Are you sure you want to change the base?
Conversation
README Add remote command documentation Makefile Add remote.c to the sources electronic_load.c Include remote.h, add remote_init and remote_timer calls uart.c / uart.h create circular buffer for receiving command over serial ui.c / ui.h move ui_event_t enum to header so it can be accessed by remote code
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.
Sorry that it took me so long to review your pull request. I added some comments. Please update your code so I can merge it.
@@ -28,7 +28,7 @@ version 3.8 or higher (3.7 sometimes crashes during compilation). | |||
The datasheet of the STM8S005 claims a write endurance of only 100 flash cycles | |||
but this is only for marketing purposes as it [contains the same die](https://hackaday.io/project/16097-eforth-for-cheap-stm8s-gadgets/log/76731-stm8l001j3-a-new-sop8-chip-and-the-limits-of-stm8flash) | |||
as the STM8S105 which is rated for 10000 cycles. So you don't have to worry | |||
about bricking your device by flashing it too often. Mine has far more than 100 | |||
about bricking you device by flashing it to often. Mine has far more than 100 |
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.
Why did you change this? The original text looks good to me.
* OFF = Deactivates the load | ||
NOTE: ON is similar to pressing the run button and will take you up one menu level. It will only enable the load if you are at the top menu level. Send multiple LOAD ON commands until you see the realtime output indicate the load is on. | ||
NOTE: OFF disables the load but does not return you to the menu. Use the hardware run button if you need to get in to the menu | ||
|
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.
Remote control should be independent of the currently selected menu.
|
||
### Component locations | ||
![Component locations](images/components.jpg) | ||
|
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.
Please don't duplicate the text from the main README.
clock_init(); | ||
gpio_init(); | ||
adc_init(); | ||
uart_init(); | ||
remote_init(); | ||
systick_init(); |
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.
Please use same indention settings as existing code.
void remote_setmode(uint8_t *data, Command *cmd) { | ||
if (strcmp(data, "CC") == 0) { | ||
settings.mode = MODE_CC; | ||
printf("Mode set to constant current\n"); |
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.
Output should easily be machine readable for remote control from scripts. Add something like "OK " in front of every response.
#define UART_BUFFER_EMPTY(uart_buffer) (uart_buffer.rdIdx == uart_buffer.wrIdx) | ||
#define UART_BUFFER_FULL(uart_buffer) ((UART_BUFFER_MASK & uart_buffer.rdIdx) == (UART_BUFFER_MASK & (uart_buffer.wrIdx+1))) | ||
#define UART_BUFFER_COUNT(uart_buffer) (UART_BUFFER_MASK & (uart_buffer.wrIdx - uart_buffer.rdIdx)) | ||
|
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.
These macros should be functions (perhaps marked as "inline")
EVENT_TIMER = 0b100000, | ||
EVENT_PREVIEW = 0b1000000, | ||
} ui_event_t; | ||
|
||
void ui_init(); |
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.
Only required in ui.c. So please leave them there.
uint32_t rdIdx; | ||
uint8_t data[UART_BUFFER_SIZE]; | ||
} uart_buffer_t; | ||
extern uart_buffer_t guart_buffer; |
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.
This is an internal buffer and should not be visible outside this unit.
void remote_init() { | ||
} | ||
|
||
void remote_timer() { |
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.
The check if there are new characters should be pretty fast. No need to run it only in a timer handler.
} | ||
|
||
void remote_timer() { | ||
//TODO: Setup pacing for large printf output to avoid Timer errors |
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.
Probably the whole systick timer stuff in main could be moved to a function which is also called in putchar().
This version of remote control moves the command processing out of the interrupt routine and in to it own files. The structure is designed to be modular, like the rest of the code, and easy to expand. Online help is available using HELP or [command] HELP. All input is validated. The responses are verbose and easy to understand.
README
Add remote command documentation
Makefile
Add remote.c to the sources
electronic_load.c
Include remote.h, add remote_init and remote_timer calls
uart.c / uart.h
create circular buffer for receiving command over serial
ui.c / ui.h
move ui_event_t enum to header so it can be accessed by remote code