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

tla2528 driver #12

Merged
merged 38 commits into from
Nov 17, 2024
Merged

Conversation

MichaelYKersey
Copy link
Contributor

@MichaelYKersey MichaelYKersey commented Oct 19, 2024

tla2528 driver and wrappers are working and tested only a few features are still to implemented:

  • reset on object creation (chip will automatically do so on power cycle)
  • ADC CRC for data validation channel id confirmation
  • ADC sample averaging for more precision (option)

void application(application_framework& p_framework)
{

hal::byte i2c_address = 0x10; // 0x10 is the tla address for no resistors
Copy link

Choose a reason for hiding this comment

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

Why must the user pass this in if this is the default? Why not make the constructor have a default parameter that sets the address to 0x10 and the dev can change this if they need to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0x10 is a default parameter. For the demos, I put the address at the top for ease of access, as that way, people aren't wondering how to change the address if they just want to test their TLA and they have it wired to a different address.

Copy link

Choose a reason for hiding this comment

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

This is probably best to remove. If the constructor doesn't describe in detail how the i2c address works, it should so anyone reading the header can see exactly when they'd need to use a differnt address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated documentation & removed address from demos, look it over

drivers/applications/tla2528_output_pin_demo.cpp Outdated Show resolved Hide resolved
drivers/src/tla2528.cpp Outdated Show resolved Hide resolved
drivers/src/tla2528.cpp Outdated Show resolved Hide resolved
Comment on lines 130 to 134
uint16_t data = 0;
hal::bit_modify(data).insert<hal::bit_mask::from(4, 11)>(data_buffer[0]);
hal::bit_modify(data).insert<hal::bit_mask::from(0, 3)>(
hal::bit_extract(hal::bit_mask::from(4, 7), data_buffer[1]));
return data / 4095.0;
Copy link

Choose a reason for hiding this comment

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

Give the bit masks names so its easier to understand what they are.
Split up the bit_extract into its own named variable so its easier to distinguish against the rest of the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated according to the conversation we had, using manual bit op and added a comment pointing to the datasheet.

drivers/src/tla2528.cpp Outdated Show resolved Hide resolved
drivers/src/tla2528_adapters.cpp Outdated Show resolved Hide resolved
@MichaelYKersey MichaelYKersey requested a review from kammce October 30, 2024 04:45
Copy link

@kammce kammce left a comment

Choose a reason for hiding this comment

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

Looks great. The last comments are for docs and TODO comments. After those are fixed up, this should be merged 😄

Great work @MichaelYKersey ! I know this took a very long time and a lot of effort!

drivers/include/tla2528.hpp Outdated Show resolved Hide resolved
drivers/include/tla2528.hpp Show resolved Hide resolved
drivers/include/tla2528.hpp Show resolved Hide resolved
drivers/include/tla2528_adapters.hpp Outdated Show resolved Hide resolved
drivers/src/tla2528.cpp Outdated Show resolved Hide resolved
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