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

changed mcp2515 driver to use nonblocking spi #817

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

kikass13
Copy link

@kikass13 kikass13 commented Feb 1, 2022

  • change mcp2515 to use proper modm::can driver api
  • change mcp2515 to use the nonblocking spi api
    • mostly, initialization routines are still blocking but i guess tahts fine
  • add example
  • utilize attachConfigurationHandler for switching device spi types
  • test example on a nucleo or discovery board
  • optional:
    • utilize attachConfigurationHandler for switching device clock speeds

@kikass13 kikass13 changed the title changed mcp2515 driver to use rx and tx buffers as well as uphold the… changed mcp2515 driver to use nonblocking spi Feb 1, 2022
…king, which should be fine because it is controlled via interrupt
Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Nice!
Thanks for taking care of this!

return true;
}

template <typename SPI, typename CS, typename INT>
modm::ResumableResult<bool>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't update() return modm::ResumableResult<void>?
Because nobody (in the main loop) will handle the return value...

Copy link
Author

Choose a reason for hiding this comment

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

it's just the return value of sendMessage indicating if a message was sent this cycle ... not really useful but i thought why not :D ... iwill remove it

Copy link
Author

Choose a reason for hiding this comment

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

it is void now

Copy link
Member

Choose a reason for hiding this comment

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

it is void now

No it's not.

src/modm/driver/can/mcp2515_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/can/mcp2515.hpp Outdated Show resolved Hide resolved
examples/stm32f4_discovery/can_m2515/main.cpp Outdated Show resolved Hide resolved
examples/stm32f4_discovery/can_m2515/main.cpp Outdated Show resolved Hide resolved
@rleh
Copy link
Member

rleh commented Feb 1, 2022

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)!
@kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

@chris-durand
Copy link
Member

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

That requires to reconfigure SPI timings when the SpiDevice acquires the SpiMaster. A method similar to the configuration handler could be used but this time it is provided externally by the user and not the driver.
The handler function pointer could be put into the SpiDevice class. It would be called inside of the SpiMaster implementation. If not used the runtime overhead of the solution would be a not taken branch with a nullptr check inside of the SpiMaster::acquire() method.

@chris-durand
Copy link
Member

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

@kikass13
Copy link
Author

kikass13 commented Feb 1, 2022

Another addition: This driver thould use the SpiDevice abstraction and should make use of void attachConfigurationHandler(Spi::ConfigurationHandler handler)! @kikass13: We need this, because we have multiple MCP2515 and one ADIS16470 connected to the same SPI.

Off-topic discussion: In the case mentioned above the SPI clock has to be limited to 1MHz or 2MHz (ADIS16470 limitation), but for the MCP2515 a higher SPI clock would be desirable. This probably affects many (SPI) drivers, what would be the most elegant way to solve this in modm?

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

@chris-durand
Copy link
Member

chris-durand commented Feb 1, 2022

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

@kikass13
Copy link
Author

kikass13 commented Feb 2, 2022

I don't know what that is bit I'll look into it tomorrow (after our work meeting stuff)

The configuration handler is actually quite simple. If you have multiple devices on one bus that require different SPI modes you'll need to reconfigure the mode whenever you run a transfer on the next device. You can define a callback that configures the required parameters and modm will call it automatically when needed. Here is an example.

EDIT: if you mean the SpiDevice with your comment, it's a class that manages shared access to one SpiMaster by many device drivers. You basically inherit your driver from SpiDevice<SpiMaster> and put RF_WAIT_UNTIL(this->acquireMaster()); before you do anything with the SPI and call releaseMaster() when you are done. That way it's made sure only one driver uses the bus at the same time. You can look at basically any other SPI based driver to see how it is used.

Yeah the second one .. you explained what it does quite well, thanks :)

@kikass13
Copy link
Author

kikass13 commented Feb 4, 2022

@rleh I have quickly hacked together a sub-optimal implementation for switching the SPI baudrate with multiple devices. This is just a rough proof of concept for STM32 only and makes changes to the SpiMaster API.

https://github.com/chris-durand/modm/tree/wip_spi_user_conf_handler

@rleh @chris-durand
i tested this and seems to work (one would have to check with an oscilloscope if it actually happened, and I don't have one at home :P)

@kikass13
Copy link
Author

kikass13 commented Feb 4, 2022

SpiDevice test seems to be broken? Or did I do something wrong?

@chris-durand
Copy link
Member

SpiDevice test seems to be broken? Or did I do something wrong?

* https://github.com/modm-io/modm/runs/5065030473?check_suite_focus=true

That is because of my experimental commit that was cherry-picked. It is only a proof of concept for STM32. Remove it and everything will be fine :)

@rleh
Copy link
Member

rleh commented Feb 8, 2022

this whole discussion is now geared towwards changing the whole device / driver api ... as I see it that is a necessary step because (as I said earlier) the api seems to be a little messy. But does that mean this has to be done in this specific pr?

This does not have to happen in this PR, in my view it is even nicer to do it in a separate PR.

@chris-durand
Copy link
Member

I guess @kikass13 might be asking if we should change the API of the MCP2515 driver, not the SpiDevice. Using SpiDevice requires an instantiated object but the driver had only static methods. Now half of them are static. This is messy. I would be in favor of changing the MCP2515 API to make it more consistent.

@kikass13
Copy link
Author

kikass13 commented Feb 8, 2022

@chris-durand currently i like the fact that this thing is static. I dislike the "mix" but i can live with it ... I have some software running this thing currently (successfully), and I really depend on the fact, that the can interfaces can be used statically.

  • i.e. just provide it as a template argument and call some functions on it (without pushing / holding) references to everything and so on

At least the current implementation works for me

  • after fixing some of the random ci errors for avr

@rleh
Copy link
Member

rleh commented Feb 8, 2022

I really depend on the fact, that the can interfaces can be used statically

Can't you in the class where you passed the static class as a template argument instantiate an object of the template argument? Then it works with both static and non-static classes.

@kikass13
Copy link
Author

kikass13 commented Feb 8, 2022

I really depend on the fact, that the can interfaces can be used statically

Can't you in the class where you passed the static class as a template argument instantiate an object of the template argument? Then it works with both static and non-static classes.

sure, i was just trying to explain that the old "design" could still be valid. @rleh you blocked my initial changes because I changed the API, so changing it back again now seems kind of redundant :D

kikass13 and others added 3 commits February 8, 2022 17:17
…t (aka NOT attempting to send it) if the mcp is not readyToSend() instead of waiting for it to be ready, which defeats the point of buffering packets indefiniteley
examples/nucleo_f439zi/can_m2515/main.cpp Outdated Show resolved Hide resolved
examples/nucleo_f439zi/can_m2515/main.cpp Outdated Show resolved Hide resolved
examples/nucleo_f439zi/can_m2515/main.cpp Outdated Show resolved Hide resolved
examples/nucleo_f439zi/can_m2515/main.cpp Outdated Show resolved Hide resolved
examples/nucleo_f439zi/can_m2515/main.cpp Outdated Show resolved Hide resolved
src/modm/driver/can/mcp2515.lb Outdated Show resolved Hide resolved
@@ -16,24 +16,13 @@
#ifndef MODM_MCP2515_HPP
#error "Don't include this file directly, use 'mcp2515.hpp' instead!"
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

Copy link
Member

Choose a reason for hiding this comment

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

Not resolved.

src/modm/driver/can/mcp2515_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/can/mcp2515_impl.hpp Outdated Show resolved Hide resolved
Comment on lines 292 to 295
if(not modm_assert_continue_ignore(rxQueue.push(messageBuffer_), "mcp2515.can.tx",
"CAN transmit software buffer overflowed!", 1)){
/// ignore
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove the if statement if the body is empty.

Copy link
Author

Choose a reason for hiding this comment

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

well it was there before ... it shows intent .. so i kept it for now

…code; minor style changes to mcp2515 driver impl
@kikass13
Copy link
Author

@rleh i have changed all the points you made (hopefully to your satisfaction) :)

…d unused project xml things; cleaned up unused headers; changed mcp2515 driver class attributes to not have _ underscore postfix
Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

  • Inside initializeWithPrescaler() SPI access is performed without locking the bus (using SpiDevice::acquireMaster()/::releaseMaster())
  • writeRegister(), readRegister() and bitModify() also perform SPI access without using SpiDevice::acquireMaster()/::releaseMaster() and are not resumable

You have to switch from the update() resumable function to a protothread, and inside the protothread you can then do the initialization stuff, triggered by a flag set from the initialize() function.

Comment on lines 5 to 6
<option name="modm:driver:mcp2515:buffer.tx">32</option>
<option name="modm:driver:mcp2515:buffer.rx">32</option>
Copy link
Member

Choose a reason for hiding this comment

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

32 is already the default value. Remove?

Copy link
Author

Choose a reason for hiding this comment

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

not a good idea, this shows intent .. nobody would know that this exists otherwise

Copy link
Member

Choose a reason for hiding this comment

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

Everybody that reads the documentation or uses lbuild discover-options will know about it...

src/modm/driver/can/mcp2515_options.hpp.in Outdated Show resolved Hide resolved
namespace mcp2515{
namespace options{

%% if options["buffer.tx"] > 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%% if options["buffer.tx"] > 0

buffer.tx is enforced to be within range 1..(2^16-2) by the lbuild module.

Copy link
Member

Choose a reason for hiding this comment

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

(Same for rx buffer)

@@ -16,24 +16,13 @@
#ifndef MODM_MCP2515_HPP
#error "Don't include this file directly, use 'mcp2515.hpp' instead!"
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Not resolved.

src/modm/driver/can/mcp2515_impl.hpp Show resolved Hide resolved
return true;
}

template <typename SPI, typename CS, typename INT>
modm::ResumableResult<bool>
Copy link
Member

Choose a reason for hiding this comment

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

it is void now

No it's not.

Comment on lines 267 to 272
template <typename SPI, typename CS, typename INT>
modm::ResumableResult<bool>
modm::Mcp2515<SPI, CS, INT>::update(){
using namespace mcp2515;

RF_BEGIN();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that rather be a Protothread and not a resumable function?

Copy link
Author

Choose a reason for hiding this comment

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

dunno what you mean. As the api is right now (static), there exists a protothread in the user code (which you told me in a prior version of this driver update, that it should be consistent to the general driver api) ... which one is it?

Copy link
Member

Choose a reason for hiding this comment

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

Hint:

Suggested change
template <typename SPI, typename CS, typename INT>
modm::ResumableResult<bool>
modm::Mcp2515<SPI, CS, INT>::update(){
using namespace mcp2515;
RF_BEGIN();
template <typename SPI, typename CS, typename INT>
void
modm::Mcp2515<SPI, CS, INT>::run()
{
PT_BEGIN();
//...

Copy link
Author

@kikass13 kikass13 Mar 18, 2022

Choose a reason for hiding this comment

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

then the user hjas two protothreads runnign inside each other?
why should the thread be in the driver instead of the user deciding when or in which context update is called?

Hint:

  • not making a point clear / answering the implicity question of what do you actually want to do and why does not help me :D

if ((readStatus(READ_STATUS) & (TXB2CNTRL_TXREQ | TXB1CNTRL_TXREQ | TXB0CNTRL_TXREQ)) ==
RF_BEGIN();

tempS = true;
Copy link
Member

Choose a reason for hiding this comment

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

Meaningful variable name or explanation please...

Comment on lines 325 to 327
ready = false;
}
return ready;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ready = false;
}
return ready;
return false;
}
return true;

@@ -313,11 +398,9 @@ void
modm::Mcp2515<SPI, CS, INT>::writeRegister(uint8_t address, uint8_t data)
{
chipSelect.reset();

Copy link
Member

Choose a reason for hiding this comment

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

Still a lot of unnecessary whitespace change in this part of the file.

@rleh
Copy link
Member

rleh commented Mar 16, 2022

@kikass13 Please stop marking my change requests which are not resolved as resolved.

@rleh rleh added this to the 2022q1 milestone Mar 16, 2022
@rleh rleh mentioned this pull request Mar 28, 2022
30 tasks
@rleh rleh modified the milestones: 2022q1, 2022q2 Mar 30, 2022
@rleh rleh modified the milestones: 2022q2, 2022q3 Jul 1, 2022
@salkinium salkinium removed this from the 2022q3 milestone Oct 1, 2022
@rleh rleh added the stale ♾ label Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants