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.

Sign up for GitHub

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

Jump to bottom

Ltc driver #23

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Ltc driver #23

merged 6 commits into from
Oct 27, 2023

Conversation

dyldonahue
Copy link
Contributor

This worked to port the LTC driver to STM HAL from NERduino. Mostly, this meant replacing NERduino SPI to STM32 HAL SPI, and adding STM32 HAL GPIO.

Please look at comments, some of them reference changes that i specifically want an extra set of eyes to confirm im not missing anything

nwdepatie
nwdepatie approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@nwdepatie nwdepatie left a comment

Choose a reason for hiding this comment

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

I think this looks good, some comments tho:

  1. Do we need to handle an errors from SPI stuff, initialization or transmission? If so we need to make functions return errors somehow.
  2. Should we have the application hold an LTC struct that we pass in to initialize + any transmission functions? I.e.
ltc6804_initialize(&ltc);
ltc6804_adax(&ltc);
etc...
  1. We prob wanna do a bit of formatting, maybe use this PR as an excuse to copy the clang-format file from shepherd into this repo? Idk how the entire file would respond to an autoformat but id at least like snake_case for function names. Feel free to disregard if you wanna make it a separate ticket/don't think its worth it

Apart from that, I think that you can merge, but def read thru and consider comments. Top priority is just getting shepherd to build and this is a big blocker

general/src/ltc68041.c Outdated
ltc_config *ltcconfig;

//TODO ensure shepherd app configures SPI with these settings:
//const SPISettings ltcSPISettings = SPISettings(1000000, MSBFIRST, SPI_MODE3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea keeping these for now. Any idea how we would configure SPI like this or nah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am assuming / somehwat confident that HAL has functions to set these up - or, it may be something to configure in the ioc (which ofc just produces the setup functions). Either way, i had already gone ahead and created a ticket in shepherd to handle this

general/src/ltc68041.c Outdated
@@ -88,10 +89,13 @@ uint8_t ADAX[2]; //!< GPIO conversion command.
the Normal ADC mode.
*/

void LTC6804_initialize()
void LTC6804_initialize(SPI_HandleTypeDef *hspi, GPIO_TypeDef *hgpio, uint8_t cs_pin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be improved by returning a pointer to the LTC struct or NULL. Having a local struct inside the driver feels funky imo but I think that both implementations will work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was debating whether to have the global struct within ltc, or return something to the application. I ended up deciding that i wanted shepherd to be as removed form this driver as possible, thus no retunr object, but i was on the fence anyway so if u prefer that im happy to change

general/src/ltc68041.c Outdated
digitalWrite(SPI1_CS, LOW);
NERduino.writeSPI1(cmd, 4, ltcSPISettings);
digitalWrite(SPI1_CS, HIGH);
HAL_GPIO_WritePin(ltcfonig->gpio, ltcconfig->cs_pin, GPIO_PIN_RESET);
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda generalizing again, do we want to have the application keep a pointer to an LTC struct? I think that would be useful in the event that there are multiple LTC chips but tbh might be overkill for our application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didnt consider multilpe, that alone might make it better to switch

general/src/ltc68041.c Outdated
digitalWrite(SPI1_CS, LOW);
NERduino.writereadSPI1(cmd,4,data,(REG_LEN*total_ic), ltcSPISettings);
digitalWrite(SPI1_CS, HIGH);
HAL_GPIO_WritePin(ltcfonig->gpio, ltcconfig->cs_pin, GPIO_PIN_RESET);
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird tab, might be good to run an autoformat on this + potentially use this PR as an opportunity to put the autoformat file into this repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i manually formatted for now, was thinking id go ahead and auto it at some point, guess now makes the most sense

general/src/ltc68041.c
set_adc(MD_NORMAL,DCP_DISABLED,CELL_CH_ALL,AUX_CH_ALL);
ltcconfig->spi = hspi;
ltcconfig->gpio = hgpio;
ltcconfig->cs_pin = cs_pin;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think important: We should set CS pin high in this init function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

general/src/ltc68041.c Outdated
digitalWrite(SPI1_CS, HIGH);

HAL_GPIO_WritePin(ltcfonig->gpio, ltcconfig->cs_pin, GPIO_PIN_RESET);
HAL_SPI_Transmit(ltcconfig->spi, cmd, CMD_LEN, HAL_MAX_DELAY);
Copy link
Contributor

Choose a reason for hiding this comment

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

General q: are there any errors from these SPI functions that we should handle in the event that they fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gonna make another ticket for this, good idea to do so but a bit bigger than i wanna address this second

nwdepatie
nwdepatie approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@nwdepatie nwdepatie left a comment

Choose a reason for hiding this comment

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

I think this works for me. Definitely an improvement from the og version of this at least

general/include/ltc68041.h

static const unsigned int crc15Table[256] = {
0x0, 0xc599, 0xceab, 0xb32, 0xd8cf, 0x1d56, 0x1664, 0xd3fd, 0xf407,
0x319e, 0x3aac, //!< precomputed CRC15 Table
Copy link
Contributor

Choose a reason for hiding this comment

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

this got weird in autoformatter

@nwdepatie nwdepatie merged commit 0870c0f into main Oct 27, 2023
1 check passed
@nwdepatie nwdepatie deleted the ltc-driver branch October 27, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nwdepatie nwdepatie nwdepatie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dyldonahue @nwdepatie