hytech-racing / MCU

https://hytech-racing.github.io/MCU/index.html
GNU General Public License v3.0
0 stars 0 forks source link

Combine enqueue functions in TelemetryInterface #40

Closed walkermburns closed 7 months ago

walkermburns commented 7 months ago

Minor issue, but there is a lot of repeated code in TelemetryInterface with respect to passing CAN messages into the circular buffer, Ideally, there should be one enqueue/circular buffer function that packs a CAN_message_t into the circular buffer (this should be made private), and the individual packing functions will create a CAN message and then send it to the singular function.

since all of these status messages are sent at the same time in the tick function, the function use could also be decreased to only use one status message send function that packs each message and loads it onto the queue, but I will leave that up to the rewriter.

At the very least, if the above is not done, the update_*() functions should just pack and push the message onto the queue using the enqueue function to decrease function and overall line count

CL16gtgh commented 7 months ago

They have different CAN messages, hince different ID's and potentially difference sizes. The amount of code that can be extracted out is limited and does not seem that critical; the seperation of update and enqueue was designed with the intention to leave the option for some level of flexibility, would you consider this unnecessary? I have not come up with a solution that has significant benefit, you're welcome to move things around though.

walkermburns commented 7 months ago

This is what I am kind of thinking of. An actually good use of templates. I have yet to run tests on this and make sure it works tho. image This would remove the need for an enqueue function for every message type although that is how I wrote it there

walkermburns commented 7 months ago

I am going to rewrite the rest of them this way and branch it off until I can test it on hardware tomorrow. I will probably put the send_all (will be renamed) into the update functions just because as a telemetry function, there is no reason to tick them separately. This is just for sending data. However, they will just be one line now, and will be easy enough to move out again.

CL16gtgh commented 7 months ago

Another concern is we were sending different data out at different rate. I'm not entirely sure why, but could make sense if some are less critical or varies more slowly. Just something to consider.

walkermburns commented 7 months ago

For right now, these are only meant to be for logging/daq purposes. Their send rate should remain the same or similar for that reason. Until there is a reason to separate them out, I think this will be sufficient. I will also use some of them on dash, but their rate is high enough for that.