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

Timestamped Frames #10

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

Timestamped Frames #10

wants to merge 28 commits into from

Conversation

equationcrunchor
Copy link
Contributor

@equationcrunchor equationcrunchor commented Apr 27, 2019

Uses HAL_GetTick to put a timestamp on received CAN messages. Includes changes from the interruptsbranch. See sensor-node-timestamped-frames branch in MY19 for example usage in sensor node. If using interrupts, one should pass the correct raw bus to CANlib_HandleFrame in the callback. This will break existing code that uses CANlib.

Copy link
Member

@nistath nistath left a comment

Choose a reason for hiding this comment

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

There are several things missing:

  1. No support for interrupt user to provide their own frame and timestamp.
  2. No support for interrupt user to provide their own HAL frame and timestamp and have an automatic conversion.
  3. Timestamp is not time of unpacking, it's time of reception.

fw('void CANlib_Handle_{}(Frame *frame)'.format(
tot_name, tot_name) + ' {\n' + '\tCANlib_Unpack_{}(frame, &CANlib_{}_Input);\n'.format(tot_name, tot_name) + '}\n\n')
fw('void CANlib_Handle_{}(Timestamped_Frame *ts_frame) {{\n'.format(tot_name, tot_name))
fw('\tCANlib_{}_Input.timestamp = HAL_GetTick();\n'.format(tot_name))
Copy link
Member

Choose a reason for hiding this comment

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

This should be coming in as a function parameter. Devices could be buffering frames on their own and have the timestamp be in the past.

src/static.h Show resolved Hide resolved
@@ -38,7 +39,7 @@ HAL_StatusTypeDef CANlib_TransmitFrame(Frame *frame, CANlib_Bus_T bus) {
return HAL_CAN_AddTxMessage(hcan, &pHeader, frame->data, &pTxMailbox);
}

void CANlib_ReadFrame(Frame *frame, CANlib_Bus_T bus) {
bool CANlib_ReadFrame(Frame *frame, CANlib_Bus_T bus) {
Copy link
Member

Choose a reason for hiding this comment

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

If it returns anything it should probably be HAL_StatusTypeDef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is HAL_CAN_GetRxMessage only returns HAL_OK or HAL_ERROR anyway; and also it could possibly not even be called if both FIFO's are empty

src/driver.h Outdated

CANlib_Transmit_Error_T CANlib_TransmitFrame(Frame *frame, CANlib_Bus_T bus);
void CANlib_ReadFrame(Frame *frame, CANlib_Bus_T bus);
bool CANlib_ReadFrame(Frame *frame, CANlib_Bus_T bus);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe introduce CANlib_Receive_Error_T, idk if we need to mess with this now tho

fw('\t}\n')

fw(
'}\n\n'
)

for busnm, bus in computer.participation['name']['can'].subscribe.items():
fw(
'static void CANlib_update_can_{}(void)'.format(busnm) + '{\n' +
Copy link
Member

Choose a reason for hiding this comment

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

should be _Update and also missing space before {.

'static void CANlib_update_can_{}(void)'.format(busnm) + '{\n' +
'\tTimestamped_Frame ts_frame;\n'
)
if any(is_multplxd(msg) for msg in bus):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here? Unused

fw('\tuint64_t bitstring;\n')

fw(
'\tif (CANlib_ReadFrame(&(ts_frame.frame), {})) {{\n'.format(busnm) +
Copy link
Member

Choose a reason for hiding this comment

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

You don't set the timestamp so it's initialized with junk. Here you can set it with get tick

fw('void CANlib_update_can() {\n')
for busnm in computer.participation['name']['can'].subscribe.keys():
fw('\tCANlib_update_can_{}();\n'.format(busnm))
fw('}\n\n')

fw('void CANlib_HandleFrame(CAN_Raw_Bus_T raw_bus) {\n')
Copy link
Member

Choose a reason for hiding this comment

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

How is this different than the update? Where can a user of interrupts provide a timestamped frame and have it accounted for?

@@ -78,28 +78,68 @@ def write(can, computers, output_path=computer_c_dir_path):

fw('\t}\n}\n\n')

fw(
'CAN_Raw_Bus_T CANlib_GetConceptualBus(CAN_Raw_Bus_T bus) {\n'
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for?

@equationcrunchor
Copy link
Contributor Author

equationcrunchor commented May 11, 2019

Made changes. Example usage in sensor-node-timestamped-frames branch of MY19.

@nistath
Copy link
Member

nistath commented Jul 24, 2019

@equationcrunchor please revisit to resolve conflicts and ping me for review?

@equationcrunchor
Copy link
Contributor Author

@nistath Conflicts resolved.

@@ -6,12 +6,14 @@
frame_handler, is_multplxd
from math import ceil

raw_bus_to_handle = {"CAN_1": "hcan1", "CAN_2": "hcan2", "CAN_3": "hcan3"}
Copy link
Member

Choose a reason for hiding this comment

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

This file must work with all drivers, even outside of STM.

Copy link
Member

Choose a reason for hiding this comment

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

These variables must be inferred from the spec.

)
fw('\t\tdefault:\n\t\t\tUNUSED(frame);\n\t\tbreak;\n')
fw('\t}\n}\n')
fw('void CANlib_HandleFrame(TimestampedFrame *ts_frame, time_t stamp, CAN_TypeDef* instance) {\n')
Copy link
Member

Choose a reason for hiding this comment

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

Why does this function take a timestamped frame AND a timestamp? Whoever calls it should just assign the stamp. Also use T const * if you don't modify things.

fw('\t}\n}\n')
fw('void CANlib_HandleFrame(TimestampedFrame *ts_frame, time_t stamp, CAN_TypeDef* instance) {\n')
if len(computer.participation['name']['can'].subscribe.keys()) > 0: # check if computer receives messages
fw('\tts_frame->stamp = stamp;\n')
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

fw('void CANlib_HandleFrame(TimestampedFrame *ts_frame, time_t stamp, CAN_TypeDef* instance) {\n')
if len(computer.participation['name']['can'].subscribe.keys()) > 0: # check if computer receives messages
fw('\tts_frame->stamp = stamp;\n')
fw('\tswitch ((intptr_t)instance) {\n')
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. The function should take a CANlib CAN_x instance. That way the driver can provide it and you don't need the raw_bus_to_instance.

fw('\t\tdefault:\n\t\t\tUNUSED(frame);\n\t\tbreak;\n')
fw('\t}\n}\n')
fw('void CANlib_HandleFrame(TimestampedFrame *ts_frame, time_t stamp, CAN_TypeDef* instance) {\n')
if len(computer.participation['name']['can'].subscribe.keys()) > 0: # check if computer receives messages
Copy link
Member

Choose a reason for hiding this comment

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

It's not this part's job to make a function unusable if nobody subscribes to it. It should be generated and just not be exposed to the user.

fw('#include "pack_unpack.h"\n')
fw('#include "canlib_{}.h"\n\n'.format(computer.name))

fw('extern CAN_HandleTypeDef hcan1;\n')
fw('extern CAN_HandleTypeDef hcan2;\n')
fw('extern CAN_HandleTypeDef hcan3;\n\n')
Copy link
Member

Choose a reason for hiding this comment

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

Not allowed.


CANlib_Transmit_Error_T CANlib_TransmitFrame(Frame *frame, CANlib_Bus_T bus);
void CANlib_ReadFrame(Frame *frame, CANlib_Bus_T bus);
bool HAL_CANlib_ReadFrame(CAN_HandleTypeDef *hcan, Frame* out);
Copy link
Member

Choose a reason for hiding this comment

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

Please conform to the format above, making a new type for receive error.

#ifdef __cplusplus
extern "C" {
#endif // __cplusplus

CANlib_Transmit_Error_T CANlib_TransmitFrame(Frame *frame, CANlib_Bus_T bus);
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here and driver.h should be included at the bottom.

@@ -5,6 +5,7 @@
#include "driver.h"
Copy link
Member

Choose a reason for hiding this comment

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

This should include the stm specific driver/inc/xxx.h

@@ -63,9 +65,16 @@ def write(can, computers, output_path=computer_c_dir_path):

with open(f_path, 'w') as f:
fw = f.write
fw('#include <time.h>\n')
fw('#include <stdbool.h>\n')
fw('#include <stm32f4xx_hal.h>\n')
Copy link
Member

Choose a reason for hiding this comment

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

Not allowed.

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