✨ Add tla2528 driver #5

MichaelYKersey · 2024-11-26T21:30:51Z

The tla2528 is an i2c-based GPIO expander and ADC mux. It has 8 pins that can each be independently configured and operated as GPIO or ADC. This is a port of the tla2528 driver and adapters from the SJSU robotics team.

Features

ADC readings
Digital input readings (bus & individual)
Digital output writing (bus & individual) (push-pull & open-drain)
Adapters for each

Missing/future/unimplemented features:

TLA reset on object creation
ADC 16-bit precision (currently 12-bit, but chip allows for more)
ADC channel verification (redundancy)

MichaelYKersey · 2024-11-26T21:56:21Z

currently, the reset command is declared as a private method but is unimplemented. Should it be removed? or is it better to leave it there for the binary interface consistency (I'm not sure how ABIs work)

kammce

Great work! Much of my comments have to do with formatting and documentation. The rest mostly looks good.

kammce · 2024-11-27T13:18:02Z

demos/applications/tla2528_analog_input.cpp

+
+#include <array>
+#include <libhal-util/bit.hpp>
+#include <libhal-util/i2c.hpp>
+#include <libhal-util/serial.hpp>
+#include <libhal-util/steady_clock.hpp>
+#include <libhal/input_pin.hpp>
+#include <libhal/units.hpp>
+#include <libhal-expander/tla2528.hpp>
+
+#include <libhal-expander/tla2528_adapters.hpp>
+#include <resource_list.hpp>


Suggested change

#include <array>

#include <libhal-util/bit.hpp>

#include <libhal-util/i2c.hpp>

#include <libhal-util/serial.hpp>

#include <libhal-util/steady_clock.hpp>

#include <libhal/input_pin.hpp>

#include <libhal/units.hpp>

#include <libhal-expander/tla2528.hpp>

#include <libhal-expander/tla2528_adapters.hpp>

#include <resource_list.hpp>

#include <array>

#include <libhal-util/bit.hpp>

#include <libhal-util/i2c.hpp>

#include <libhal-util/serial.hpp>

#include <libhal-util/steady_clock.hpp>

#include <libhal/input_pin.hpp>

#include <libhal/units.hpp>

#include <libhal-expander/tla2528.hpp>

#include <libhal-expander/tla2528_adapters.hpp>

#include <resource_list.hpp>

Suggestion made based on https://libhal.github.io/4.1/contributor_guide/style/#s143-include-order.

kammce · 2024-11-27T13:31:44Z

demos/applications/tla2528_analog_input.cpp

+    }
+    hal::delay(steady_clock, 500ms);
+  }
+}


All files should end with a newline. I'm adding a patch to have CI check demos directory files as well.

kammce · 2024-11-27T13:34:00Z

demos/applications/tla2528_input_pin.cpp

+#include <array>
+#include <libhal-util/bit.hpp>
+#include <libhal-util/i2c.hpp>
+#include <libhal-util/serial.hpp>
+#include <libhal-util/steady_clock.hpp>
+#include <libhal/input_pin.hpp>
+#include <libhal/units.hpp>
+#include <libhal-expander/tla2528.hpp>
+#include <libhal-expander/tla2528_adapters.hpp>
+#include <resource_list.hpp>
+


Suggested change

#include <array>

#include <libhal-util/bit.hpp>

#include <libhal-util/i2c.hpp>

#include <libhal-util/serial.hpp>

#include <libhal-util/steady_clock.hpp>

#include <libhal/input_pin.hpp>

#include <libhal/units.hpp>

#include <libhal-expander/tla2528.hpp>

#include <libhal-expander/tla2528_adapters.hpp>

#include <resource_list.hpp>

#include <array>

#include <libhal-util/bit.hpp>

#include <libhal-util/i2c.hpp>

#include <libhal-util/serial.hpp>

#include <libhal-util/steady_clock.hpp>

#include <libhal/input_pin.hpp>

#include <libhal/units.hpp>

#include <libhal-expander/tla2528.hpp>

#include <libhal-expander/tla2528_adapters.hpp>

#include <resource_list.hpp>

Do this everywhere else as well.

kammce · 2024-11-27T13:36:24Z

include/libhal-expander/tla2528_adapters.hpp

+/**
+ * @brief create a hal::input_pin driver using the tla2528 driver
+ *
+ * @param p_channel pin acting as an input pin
+ *
+ * @param p_settings input pin settings, default is no resistor
+ *
+ * @throws hal::argument_out_of_domain - if p_channel out of range (>7)
+ *
+ * @throws hal::resource_unavailable_try_again - if an adapter has already been
+ * been made for the pin
+ *
+ * @throws hal::no_such_device - no device responded on i2c bus
+ */


Suggested change

/**

* @brief create a hal::input_pin driver using the tla2528 driver

*

* @param p_channel pin acting as an input pin

*

* @param p_settings input pin settings, default is no resistor

*

* @throws hal::argument_out_of_domain - if p_channel out of range (>7)

*

* @throws hal::resource_unavailable_try_again - if an adapter has already been

* been made for the pin

*

* @throws hal::no_such_device - no device responded on i2c bus

*/

/**

* @brief create a hal::input_pin driver using the tla2528 driver

*

* @param p_channel pin acting as an input pin

* @param p_settings input pin settings, default is no resistor

* @throws hal::argument_out_of_domain - if p_channel out of range (>7)

* @throws hal::resource_unavailable_try_again - if an adapter has already been

* been made for the pin

* @throws hal::no_such_device - no device responded on i2c bus

*/

I find the spacing harder to read than keeping everything together. Can you make this change to other comments like this as well.

kammce · 2024-11-27T13:39:00Z

include/libhal-expander/tla2528_adapters.hpp

+ */
+tla2528_input_pin make_input_pin(tla2528& p_tla2528,
+                                 hal::byte p_channel,
+                                 hal::input_pin::settings const& p_settings);


Suggested change

hal::input_pin::settings const& p_settings);

hal::input_pin::settings const& p_settings = {.resistor = none});

May be a nice QOL feature to have this have a default settings parameter vs having the user have to supply each.

kammce · 2024-11-27T14:11:35Z

src/tla2528.cpp

+float tla2528::get_analog_in(hal::byte p_channel)
+{
+  set_analog_channel(p_channel);
+  // TODO: look into averaging & channel validation


Suggested change

// TODO: look into averaging & channel validation

// TODO(#issue): look into averaging & channel validation

Make an issue for this.

kammce · 2024-11-27T14:14:03Z

src/tla2528_adapters.cpp

+  if (hal::bit_extract(hal::bit_mask::from(m_channel),
+                       m_tla2528->m_object_created)) {
+    throw hal::resource_unavailable_try_again(this);
+  }


You use this same logic a few times in the adaptors. Would make sense to make a helper function for this.

kammce · 2024-11-27T14:17:25Z

src/tla2528.cpp

+  : m_i2c_bus(p_i2c)
+  , m_i2c_address(p_i2c_address)
+{
+  // TODO: reset command


Suggested change

// TODO: reset command

// TODO(#issue-number): reset command

Make an technical debt issue for this. Also you might as well call reset() here and define it with the TODO. That way the code is already in place and the empty function has the todo embedded in it.

kammce · 2024-11-27T14:18:51Z

demos/applications/tla2528_output_pin.cpp

+  hal::byte counter =
+    0;  // output counts in binary to go though all out put combinations


Suggested change

hal::byte counter =

0; // output counts in binary to go though all out put combinations

// output counts in binary to go though all out put combinations

hal::byte counter = 0;

The newline 0 looks really odd. You can put the comment one line above.

kammce · 2024-11-27T14:19:46Z

src/tla2528_adapters.cpp

+tla2528_output_pin::~tla2528_output_pin()
+{
+  hal::bit_modify(m_tla2528->m_object_created)
+    .set(hal::bit_mask::from(m_channel));


Suggested change

.set(hal::bit_mask::from(m_channel));

.clear(hal::bit_mask::from(m_channel));

Shouldn't this be clear?

Do the same for the others.

kammce · 2024-11-27T14:22:22Z

currently, the reset command is declared as a private method but is unimplemented. Should it be removed? or is it better to leave it there for the binary interface consistency (I'm not sure how ABIs work)

See my comment on what you should do about reset. As for ABI, class functions do not effect the ABI of a function so nothing to worry about from that angle.

MichaelYKersey added 4 commits November 26, 2024 11:04

copy tla2528 from SJSU-robotics

5647074

adapted to be buildable

60fb10f

tla2528.test.cpp

335ca66

tla 2528 datasheet

4cdb5ad

kammce changed the title ~~Tla2528~~ ✨ Add tla2528 driver Nov 27, 2024

kammce requested changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add tla2528 driver #5

✨ Add tla2528 driver #5

MichaelYKersey commented Nov 26, 2024

MichaelYKersey commented Nov 26, 2024

kammce left a comment

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce Nov 27, 2024

kammce commented Nov 27, 2024

	hal::input_pin::settings const& p_settings);
	hal::input_pin::settings const& p_settings = {.resistor = none});

	// TODO: look into averaging & channel validation
	// TODO(#issue): look into averaging & channel validation

		hal::byte counter =
		0; // output counts in binary to go though all out put combinations

	.set(hal::bit_mask::from(m_channel));
	.clear(hal::bit_mask::from(m_channel));

✨ Add tla2528 driver #5

Are you sure you want to change the base?

✨ Add tla2528 driver #5

Conversation

MichaelYKersey commented Nov 26, 2024

MichaelYKersey commented Nov 26, 2024

kammce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kammce commented Nov 27, 2024