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

[driver] Adding AS5047 driver and example #1138

Merged
merged 1 commit into from Mar 8, 2024

Conversation

hshose
Copy link
Contributor

@hshose hshose commented Mar 6, 2024

  • driver
  • example
  • tested on hardware

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Nice, just some nitpicks.

par ^= par >> 2;
par ^= par >> 1;
return ((par & 1) << 15) | (0x7fff & num);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is the modm namespace which is quite generic. Would be good to move to struct as5047 or put into the relevant :math module.

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'm not sure, how to move it into the struct?

If I declare `setMsbToEvenParity` as `static constexpr` in the `as5047` struct, I get an error saying `error: 'static constexpr uint16_t modm::as5047::setMsbToEvenParity(uint16_t)' used before its definition`
struct as5047
{
	enum class Errorfl : uint16_t
	{
		Parerr = Bit2,
		Invcomm = Bit1,
		Frerr = Bit0,
	};
	MODM_FLAGS16(Errorfl)

	enum class Prog : uint16_t
	{
		Progver = Bit6,
		Progotp = Bit3,
		Otpref = Bit2,
		Progen = Bit0,
	};
	MODM_FLAGS16(Prog)

	enum class Diaagc : uint16_t
	{
		Magl = Bit11,
		Magh = Bit10,
		Cof = Bit9,
		Lf = Bit8,
	};
	MODM_FLAGS16(Diaagc)

	static constexpr uint16_t
	setMsbToEvenParity(const uint16_t num)
	{
		uint16_t par = 0x7fff & num;
		par ^= par >> 8;
		par ^= par >> 4;
		par ^= par >> 2;
		par ^= par >> 1;
		return ((par & 1) << 15) | (0x7fff & num);
	}

	enum class Register : uint16_t
	{
		ReadNop = setMsbToEvenParity(((1 << 14) | 0x0000)),
		ReadErrfl = setMsbToEvenParity(((1 << 14) | 0x0001)),
		ReadProg = setMsbToEvenParity(((1 << 14) | 0x0003)),
		ReadDiaagc = setMsbToEvenParity(((1 << 14) | 0x3FFC)),
		ReadMag = setMsbToEvenParity(((1 << 14) | 0x3FFD)),
		ReadAngleunc = setMsbToEvenParity(((1 << 14) | 0x3FFE)),
		ReadAnglecom = setMsbToEvenParity(((1 << 14) | 0x3FFF)),

	};

	struct modm_packed Data
	{
		/// @return
		constexpr float
		getAngleRad() const
		{
			const uint16_t angle = data & 0x3fff;
			return static_cast<float>(angle) / 16383.f * 2.f * std::numbers::pi_v<float>;
		}

		/// @return
		constexpr float
		getAngleDeg() const
		{
			const uint16_t angle = data & 0x3fff;
			return static_cast<float>(angle) / 16383.f * 360.f;
		}

		/// @return
		constexpr uint16_t
		getAngleRaw() const
		{
			const uint16_t angle = data & 0x3fff;
			return angle;
		}

		uint16_t data;
	};
};  // struct as5047

I think this might be related to this:

The Standard should make clear that a constexpr member function cannot be used in a constant expression until its class is complete.

Do you have any suggestions?
Should I put everything into an as5047 namespace?

I don't think moving the function as is to the math: is appropriate, since the function specifically calculates and sets bit 15 of a uint16_t, if moved to math:, a more generic version might be more appropriate. Also, other modm modules like etlcpp have their own functions to calculate parity bits...

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, C++ amiright!1!!

You can put it into a modm::details namespace then and use /// @cond and /// @endcond to block doxypress from indexing it (Examples). Prolly prefix the function with as5047_ to avoid collisions if you wanna be super careful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, C++ amiright!1!!

🐈 No we are super careful 🐱

src/modm/driver/encoder/as5047.lb Outdated Show resolved Hide resolved
src/modm/driver/encoder/as5047.hpp Outdated Show resolved Hide resolved
src/modm/driver/encoder/as5047_impl.hpp Outdated Show resolved Hide resolved
src/modm/driver/encoder/as5047_impl.hpp Outdated Show resolved Hide resolved
@salkinium salkinium added this to the 2024q1 milestone Mar 6, 2024
@hshose hshose force-pushed the feature/as5047 branch 2 times, most recently from ad12c38 to dd24d53 Compare March 7, 2024 22:18
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Thanks!

@salkinium salkinium merged commit dbfd93b into modm-io:develop Mar 8, 2024
12 checks passed
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.

None yet

2 participants