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

[SOFT 421] Transmit motor temp #408

Merged
merged 18 commits into from
Aug 28, 2021
Merged

[SOFT 421] Transmit motor temp #408

merged 18 commits into from
Aug 28, 2021

Conversation

YiJie-Zhu
Copy link
Contributor

Half-finished code for transmitting motor temperature through CAN.

@ryandancy ryandancy marked this pull request as draft July 7, 2021 22:37
@ryandancy ryandancy marked this pull request as ready for review July 24, 2021 17:30
@ryandancy ryandancy requested a review from hewittmcg July 28, 2021 22:58
Copy link
Contributor

@hewittmcg hewittmcg left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to work on finishing this, Frank! It looks good overall, and I like the idea of moving the temperature checking logic into its own module. Some of my comments are minor issues -- the general important things are:

  • Make sure to write unit tests so that we know the module is working as expected. These will likely be similar to the ones in test_mci_broadcast.c, so you can use that as a reference.
  • Change how the messages get received from the motor controllers so that the handling functions get called from mci_broadcast.c.
  • Figure out a way to differentiate between the different temperature readings when broadcasting over CAN so that we can easily process them when we receive a SYSTEM_CAN_MESSAGE_MOTOR_TEMPS message

I mentioned the possibility of moving things into mci_broadcast.c, but that's only if it makes life easier for you 😃

projects/mci/inc/motor_temperature.h Outdated Show resolved Hide resolved
projects/mci/inc/motor_temperature.h Outdated Show resolved Hide resolved
projects/mci/inc/motor_temperature.h Outdated Show resolved Hide resolved
projects/mci/src/motor_temperature.c Outdated Show resolved Hide resolved
projects/mci/src/motor_temperature.c Outdated Show resolved Hide resolved
projects/mci/src/motor_temperature.c Outdated Show resolved Hide resolved
projects/mci/src/motor_temperature.c Outdated Show resolved Hide resolved
Comment on lines 78 to 92
status_ok_or_return(generic_can_register_rx(
setting->motor_can, prv_handle_sink_temperature_rx, GENERIC_CAN_EMPTY_MASK,
MOTOR_CAN_RIGHT_SINK_MOTOR_TEMPERATURE_FRAME_ID, false, storage));

status_ok_or_return(generic_can_register_rx(
setting->motor_can, prv_handle_sink_temperature_rx, GENERIC_CAN_EMPTY_MASK,
MOTOR_CAN_LEFT_SINK_MOTOR_TEMPERATURE_FRAME_ID, false, storage));

status_ok_or_return(generic_can_register_rx(
setting->motor_can, prv_handle_dsp_temperature_rx, GENERIC_CAN_EMPTY_MASK,
MOTOR_CAN_RIGHT_DSP_TEMPERATURE_FRAME_ID, false, storage));

status_ok_or_return(generic_can_register_rx(
setting->motor_can, prv_handle_dsp_temperature_rx, GENERIC_CAN_EMPTY_MASK,
MOTOR_CAN_LEFT_DSP_TEMPERATURE_FRAME_ID, false, storage));
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't actually use generic CAN for this (anymore). Instead, there's a function in mci_broadcast.c, prv_process_rx, that routes the messages we get from the WaveSculptor to the correct handling function.

There are stubs to handle motor temperature measurements in mci_broadcast.c already: prv_handle_motor_temp_rx and prv_handle_dsp_temp_rx. You can use these by making prv_handle_dsp_temperature_rx and prv_handle_sink_temperature_rx public and either:

  • Calling them from the above mci_broadcast.c functions directly, or
  • Removing the above mci_broadcast.c functions, and updating the s_cb_storage array to point to your functions instead.

The second option is a bit cleaner, but it's your call.

Note that this probably makes unit testing a bit annoying to get working, so you might want to move everything into mci_broadcast.c similar to how we handle velocity/status/bus measurements already. Then you can just update the mci_broadcast unit tests to also check for temperature instead of having to write your own test sequence. This also comes with the advantage of being able to broadcast your measurements in the mci_broadcast.c prv_periodic_broadcast_tx instead of needing a separate function/soft timer.

projects/mci/src/motor_temperature.c Outdated Show resolved Hide resolved
projects/mci/inc/motor_temperature.h Outdated Show resolved Hide resolved
@Frank-Yan Frank-Yan changed the title Half finished rx and tx of motor temp [SOFT 421] Transmit motor temp Aug 1, 2021
@github-actions
Copy link

Uncommitted codegen changes detected!

It looks like the generated header files in libraries/codegen-tooling don't match the CAN message definition file at codegen/can_messages.asciipb. If you've changed the CAN message definitions, please run make codegen locally and push the changes.

Alternatively, maybe you've edited can_messages.asciipb in the codegen-tooling-msxiv repo. This has been deprecated: please make your changes to codegen/can_messages.asciipb instead.

Copy link
Collaborator

@ryandancy ryandancy left a comment

Choose a reason for hiding this comment

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

Looks good to me! Will merge once @hewittmcg approves

Copy link
Contributor

@hewittmcg hewittmcg left a comment

Choose a reason for hiding this comment

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

One really minor nitpick, LGTM otherwise!

projects/mci/inc/mci_broadcast.h Outdated Show resolved Hide resolved
Copy link
Contributor

@hewittmcg hewittmcg left a comment

Choose a reason for hiding this comment

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

LGTM!

@ryandancy ryandancy merged commit 7805bd4 into master Aug 28, 2021
@ryandancy ryandancy deleted the soft_421_motor_temp branch August 28, 2021 16:39
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.

4 participants